CNV-87995 Add Cypress E2E tests for NNCP, Physical networks, VM networks, and NNS#228
Open
lkladnit wants to merge 1 commit into
Open
CNV-87995 Add Cypress E2E tests for NNCP, Physical networks, VM networks, and NNS#228lkladnit wants to merge 1 commit into
lkladnit wants to merge 1 commit into
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR introduces a complete Cypress end-to-end test suite for the NMState console plugin. It provides test infrastructure, configuration, support utilities, and comprehensive test coverage for Physical networks, NodeNetworkConfigurationPolicy, NodeNetworkState/Topology, and Virtual machine networks features, along with login and navigation flows. ChangesCypress E2E Test Framework and Suites
🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 13 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (13 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: lkladnit The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Author
|
/assign @lkladnit |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (11)
ui-tests-cy/support/selectors.ts (1)
69-77: 💤 Low valueUse descriptive variable names.
The variable
t_ois an abbreviation and reduces readability. Consider usingtimeoutortimeoutValueinstead.♻️ Proposed fix
Cypress.Commands.add('checkTitle', (title: string, timeout?: number) => { - const t_o = timeout ? timeout : MINUTE * 3; - cy.contains('h1', title, { timeout: t_o }).should('exist'); + const timeoutValue = timeout ?? MINUTE * 3; + cy.contains('h1', title, { timeout: timeoutValue }).should('exist'); }); Cypress.Commands.add('checkSubTitle', (subTitle: string, timeout?: number) => { - const t_o = timeout ? timeout : MINUTE * 3; - cy.contains('h2', subTitle, { timeout: t_o }).should('exist'); + const timeoutValue = timeout ?? MINUTE * 3; + cy.contains('h2', subTitle, { timeout: timeoutValue }).should('exist'); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui-tests-cy/support/selectors.ts` around lines 69 - 77, The local variable name t_o in the Cypress custom commands for checkTitle and checkSubTitle is abbreviated and harms readability; rename t_o to a descriptive name like timeoutValue (or timeout) in both functions, update the assignment (const timeoutValue = timeout ? timeout : MINUTE * 3) and replace all uses of t_o with timeoutValue so the commands Cypress.Commands.add('checkTitle', ...) and Cypress.Commands.add('checkSubTitle', ...) remain functionally identical but clearer.ui-tests-cy/support/index.ts (1)
10-18: 💤 Low valueReturn value missing when suppressing logs.
When
displayNameis'fetch'or'xhr', the function returnsundefinedinstead of a log object. While this may work in practice, it breaks the expected return contract ofCypress.log. Consider returningnullor a stub log object to avoid potential downstream issues.♻️ Proposed fix
if (Cypress.env('HIDE_XHR')) { const origLog = Cypress.log; Cypress.log = function (opts, ...other) { if (opts.displayName === 'fetch' || opts.displayName === 'xhr') { - return; + return null; } return origLog(opts, ...other); }; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui-tests-cy/support/index.ts` around lines 10 - 18, The custom Cypress.log override suppresses entries for displayName 'fetch' or 'xhr' but returns undefined, violating the expected return contract; update the override in the Cypress.log function (the override referencing origLog and opts.displayName) to return a safe value when suppressing logs—either return null or a minimal stub log object with the same shape the caller expects (e.g., id/message properties) instead of returning undefined so downstream code receives a predictable return value.ui-tests-cy/.eslintrc (1)
1-15: ⚡ Quick winConsider adding import sorting to align with the main project's linting standards.
The main project enforces import sorting via
simple-import-sort(per CONTRIBUTING.md), but this Cypress ESLint config doesn't include it. This could lead to inconsistent import organization between source code and test files.♻️ Proposed addition
{ "env": { "cypress/globals": true, "node": true }, "extends": ["plugin:cypress/recommended"], - "plugins": ["cypress"], + "plugins": ["cypress", "simple-import-sort"], "rules": { "no-console": "off", "no-namespace": "off", "no-redeclare": "off", "`@typescript-eslint/no-var-requires`": "off", - "`@typescript-eslint/no-namespace`": "off" + "`@typescript-eslint/no-namespace`": "off", + "simple-import-sort/imports": "error", + "simple-import-sort/exports": "error" } }Note: This requires adding
eslint-plugin-simple-import-sorttoui-tests-cy/package.jsondevDependencies.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui-tests-cy/.eslintrc` around lines 1 - 15, Add import-sorting to the Cypress ESLint config by installing eslint-plugin-simple-import-sort as a devDependency in ui-tests-cy/package.json and then update .eslintrc to include "plugin:simple-import-sort/recommended" in the "extends" array (and/or add "simple-import-sort" to "plugins") so that the simple-import-sort rules are applied to the Cypress tests; ensure the rule set is consistent with the main project's configuration and don't remove existing rules like "no-console" or "`@typescript-eslint/no-namespace`".Source: Coding guidelines
ui-tests-cy/views/physicalNetwork.ts (2)
17-20: ⚡ Quick winRemove redundant hardcoded wait.
Line 18 introduces a 2-second hardcoded wait immediately after Line 17 already waits for the input to be visible. This is redundant and can slow tests unnecessarily. Cypress's
.should('be.visible')already includes retry logic.♻️ Proposed fix
// Step 1: Network identity cy.get(physicalNetworkNameInput, { timeout: MINUTE }).should('be.visible'); - cy.wait(2 * SECOND); cy.get(physicalNetworkNameInput).clear(); cy.get(physicalNetworkNameInput).type(networkName);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui-tests-cy/views/physicalNetwork.ts` around lines 17 - 20, Remove the redundant hardcoded wait: after confirming the input is visible with cy.get(physicalNetworkNameInput, { timeout: MINUTE }).should('be.visible'), delete the cy.wait(2 * SECOND) call and proceed to clear and type into the element (the subsequent cy.get(physicalNetworkNameInput).clear() and .type(networkName)); this relies on Cypress's retry behavior on the visible assertion and avoids unnecessary test slowdown.
36-39: ⚡ Quick winDocument missing Step 4 and replace hardcoded wait with assertion.
The comment jumps from "Step 3" to "Step 5" without documenting Step 4. Additionally, Line 39's hardcoded 3-second wait should be replaced with an assertion-based wait for the expected post-creation state (e.g., navigation completion or success message).
♻️ Proposed improvements
// Step 3: Uplink connection cy.get('.pf-v6-c-wizard__main', { timeout: MINUTE }).should('contain', 'Uplink'); cy.contains(submitBtn, 'Next').click(); + // Step 4: [Document what step 4 does, or confirm it's skipped] + // Step 5: Review and Create cy.contains('button', 'Create network', { timeout: MINUTE }).should('be.visible'); cy.contains('button', 'Create network').click(); - cy.wait(3 * SECOND); + // Consider waiting for a success indicator or navigation instead of fixed timeout🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui-tests-cy/views/physicalNetwork.ts` around lines 36 - 39, The test jumps from "Step 3" to "Step 5" and uses a hardcoded cy.wait(3 * SECOND); update the comments to add a "Step 4: Confirm creation initiated" line and replace the fixed wait with an assertion-based wait that verifies the post-create state (for example, after clicking the 'Create network' button use cy.contains('Network created', { timeout: MINUTE }).should('be.visible') or assert navigation with cy.url().should('include', '/networks') or another app-specific success indicator); keep the existing selectors ('Create network' button, MINUTE, SECOND) but remove the static wait and assert the actual success condition instead.ui-tests-cy/views/nncp.ts (1)
19-31: ⚡ Quick winReplace hardcoded waits and improve error detection.
Lines 23 and 25 use hardcoded waits (
cy.wait(3 * SECOND)andcy.wait(5 * SECOND)), and Line 27 checks the entire page body for "already exists". This pattern is fragile and can cause flakiness.♻️ Proposed improvements
export const createNNCPFromYAML = () => { cy.get(itemCreateBtn).click(); cy.contains('button', 'With YAML').click(); cy.url({ timeout: MINUTE }).should('include', '~new'); - cy.wait(3 * SECOND); cy.get('button').contains('Create').click(); - cy.wait(5 * SECOND); - cy.get('body').then(($body) => { - if ($body.text().includes('already exists')) { + // Wait for either success navigation or error alert + cy.get('body', { timeout: 10000 }).then(($body) => { + if ($body.find('[role="alert"]').text().includes('already exists')) { cy.contains('button', 'Cancel').click(); } }); };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui-tests-cy/views/nncp.ts` around lines 19 - 31, The createNNCPFromYAML helper uses fragile hardcoded waits and a broad page-text check for duplicate errors; replace cy.wait(3 * SECOND) and cy.wait(5 * SECOND) with targeted assertions/commands that wait for specific elements or network calls (e.g., wait for the YAML modal to be visible, wait for the Create button to become enabled, or intercept the create request and wait on its route), and replace the body text check with a scoped assertion that looks for the precise duplicate error element or toast (search within the modal or the notification container) and then click the Cancel button if that specific error is present; update references in createNNCPFromYAML and usages of itemCreateBtn/Create button selectors accordingly.ui-tests-cy/views/vmNetwork.ts (2)
20-44: ⚡ Quick winReplace hardcoded waits with assertions.
Lines 23, 38, 51, 53, and 55 use hardcoded
cy.wait()calls with fixed timeouts. This is a Cypress anti-pattern that can lead to flaky tests (too short) or unnecessarily slow tests (too long). Replace with assertion-based waiting for specific UI states.♻️ Examples of assertion-based alternatives
cy.get(itemCreateBtn).click(); cy.contains('h1', 'Create virtual machine network', { timeout: MINUTE }).should('exist'); - cy.wait(2 * SECOND); cy.get(vmnNameInput, { timeout: MINUTE }) .should('be.visible')cy.contains(submitBtn, 'Create').click(); - cy.wait(5 * SECOND); cy.get('body').then(($body) => {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui-tests-cy/views/vmNetwork.ts` around lines 20 - 44, The createVMNetwork function uses hardcoded cy.wait() calls; replace them with assertions that wait for specific UI states: after opening the create dialog assert the modal header appears (already present) and wait for vmnNameInput to be visible before interacting instead of wait(2*SECOND); after clicking Next assert the next step or a form section (e.g., project mapping controls or a stepper indicator) is visible instead of wait(5*SECOND); after clicking Create assert success or error toast/text (e.g., look for confirmation message or for the "already exists" text) and if the "already exists" text is present call navigateToVMN(); remove all cy.wait() calls and use should('be.visible'), contains(...).should('exist'), or intercept+wait for network responses as appropriate to ensure deterministic timing while keeping selectors like itemCreateBtn, vmnNameInput, submitBtn, and navigateToVMN referenced for locating changes.
39-43: ⚡ Quick winScope error detection to alert elements.
Lines 39-43 check the entire page body for "already exists". Similar to the pattern in
nncp.ts, this is fragile and could match unrelated text.♻️ Proposed improvement
cy.get('body').then(($body) => { - if ($body.text().includes('already exists')) { + // Check for error in alert/notification area instead of entire body + if ($body.find('[role="alert"]').text().includes('already exists')) { navigateToVMN(); } });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui-tests-cy/views/vmNetwork.ts` around lines 39 - 43, The test is scanning the whole page body for the string "already exists", which can produce false positives; change the check to only inspect alert elements (e.g., use selectors like [role="alert"], .pf-c-alert or the same alert selector used in nncp.ts) instead of cy.get('body'), then call navigateToVMN() only if an alert's text includes "already exists" so the match is limited to real UI alerts.research-flakiness.sh (2)
58-58: 💤 Low valueInteger division truncates failure rate.
Line 58 uses integer arithmetic
$((FAIL_COUNT * 100 / TOTAL_RUNS))which truncates to whole percentages. WithTOTAL_RUNS=10, 1 failure shows as 10%, but if this is later changed to 100 runs, 1 failure would show as 1% instead of 1.0%.♻️ Proposed fix for precise percentage
-- **Failure rate:** $((FAIL_COUNT * 100 / TOTAL_RUNS))% +- **Failure rate:** $(awk "BEGIN {printf \"%.1f\", ($FAIL_COUNT * 100.0 / $TOTAL_RUNS)}")%🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@research-flakiness.sh` at line 58, The current expression $((FAIL_COUNT * 100 / TOTAL_RUNS)) uses integer division and truncates percentages; replace it by computing a floating-point percentage (e.g., using awk or bc with a set scale) and format it to at least one decimal place, store that result in a variable like FAILURE_RATE, and then use that variable in the markdown line instead of the integer expression so the displayed failure rate shows decimal precision.
18-20: 💤 Low valueAdd validation before destructive operations.
Line 18 runs
rm -rfwithout checking if the directory exists, and Line 20 invokes./test-cypress.shwithout validating it exists or is executable. While not critical, adding guards improves robustness.🛡️ Proposed validation guards
+ if [ ! -x "./test-cypress.sh" ]; then + echo "Error: test-cypress.sh not found or not executable" + exit 1 + fi + - rm -rf ui-tests-cy/gui-test-screenshots/* + if [ -d "ui-tests-cy/gui-test-screenshots" ]; then + rm -rf ui-tests-cy/gui-test-screenshots/* + fi ./test-cypress.shNote: The validation for
test-cypress.shshould be before the loop (line 13), not inside it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@research-flakiness.sh` around lines 18 - 20, Before running destructive removal and launching the test script, add existence and executability checks: verify the directory "ui-tests-cy/gui-test-screenshots" exists (e.g., test -d) before running rm -rf on its contents, and verify "./test-cypress.sh" exists and is executable (e.g., test -x or test -f and chmod +x) before invoking it; perform the "./test-cypress.sh" validation once before entering the loop referenced near the existing loop start (the comment mentioned line 13) and fail-fast with a clear error message if checks fail.ui-tests-cy/tests/nmstate/nns.cy.ts (1)
13-13: ⚡ Quick winReplace fixed sleeps with condition-based waits.
cy.wait(...)here will make this suite slower and flaky under load. Prefer waiting on concrete UI state transitions (target element/attribute change) instead of fixed time delays.Also applies to: 25-25, 30-30
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui-tests-cy/tests/nmstate/nns.cy.ts` at line 13, Replace the fixed sleep cy.wait(2 * SECOND) (and the other occurrences noted) with a condition-based Cypress wait that targets the specific UI element/state change instead of a timed delay: locate the test steps surrounding cy.wait(2 * SECOND), identify the DOM selector or text that indicates the app progressed (e.g., a button enabling/disabling, a status text change, or an element appearing/disappearing), and replace the sleep with an assertion like cy.get(<selector>, { timeout: <ms> }).should(<expectedCondition>) or cy.contains(<text>, { timeout: <ms> }).should(<expectedCondition>) so the test waits until the real UI state is reached rather than a fixed interval; apply the same replacement for the other cy.wait occurrences referenced in the comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cleanup.sh`:
- Around line 7-12: The cleanup function uses a different default TEST_NS and
omits namespace scoping for the virtualmachinenetwork delete; change TEST_NS
default to match setup.sh's value (cy-test-ns) and ensure all oc delete
invocations in cleanup (including the virtualmachinenetwork delete and the first
oc delete of nncp resources) explicitly target the namespace by adding -n
"$TEST_NS" (or equivalent) so cleanup consistently operates in the intended
namespace; update references to TEST_NS in the cleanup() function accordingly.
In `@setup.sh`:
- Around line 7-11: The TEST_NS defaulting is using substring expansion
(${TEST_NS:cy-test-ns}) which is wrong; update the variable expansion to use the
default-value operator so TEST_NS falls back when unset or null (replace the
current expansion with the correct form), leaving the setup() function and the
oc get/create logic unchanged; look for the TEST_NS definition and change it to
use the ":-" default operator (TEST_NS=${TEST_NS:-cy-test-ns}) so the namespace
creation in setup() works reliably.
In `@test-cypress.sh`:
- Around line 3-4: The script currently disables immediate exit on error with
"set +e" and never returns the actual Cypress/test exit status; update the
control flow to capture and propagate failures by recording the exit code of
setup and cypress runs into a variable (e.g., test_exit_code) after each
critical command (the Cypress invocation and any setup steps referenced in lines
22-27 and 32-40), and add a final "exit $test_exit_code" at the end so the
script exits non-zero when any step failed; ensure you still allow teardown if
needed but preserve and return the first non-zero exit code from the named
variable.
In `@ui-tests-cy/package.json`:
- Around line 13-23: The devDependencies in ui-tests-cy package.json are using
caret ranges which reduces reproducibility; update the dependency entries for
"`@cypress/webpack-preprocessor`", "cypress", "cypress-multi-reporters", "dotenv",
"esbuild-loader", "mocha-junit-reporter", "mochawesome", "mochawesome-merge",
"mochawesome-report-generator", "typescript", and "webpack" to pin exact
versions by removing the leading '^' from each version string so the
package.json lists exact versions.
In `@ui-tests-cy/plugins/index.ts`:
- Line 28: The code sets config.env.HIDE_XHR to the raw process.env.HIDE_XHR
string which treats "false" as truthy; update the assignment in plugins/index.ts
so that the value is normalized to a boolean (e.g., convert process.env.HIDE_XHR
to true only when it equals the string "true", otherwise false, and default to
false if undefined) before assigning to config.env.HIDE_XHR so Cypress receives
a real boolean.
In `@ui-tests-cy/support/commands.ts`:
- Around line 12-24: The deleteResource command builds shell strings with
unvalidated kind/name/namespace and calls cy.exec, causing a command-injection
risk; fix it by validating each parameter (kind, name, namespace) with a strict
whitelist regex (e.g., alphanumerics, dashes, dots, and underscores only) inside
the Cypress.Commands.add('deleteResource'...) function and reject or throw on
invalid values, then call cy.exec only with validated values (still using the
same oc delete invocation) to ensure no unsafe characters can be injected;
reference the deleteResource function and the cy.exec call to locate where to
add validation.
In `@ui-tests-cy/support/selectors.ts`:
- Around line 29-34: The custom Cypress command byTestID currently calls
cy.get(`[data-test="${selector}"]`, options) but does not return it, breaking
Cypress chaining; update the implementation of byTestID to return the result of
cy.get(...) so callers like cy.byTestID('foo').click() continue to work, i.e.,
ensure the function registered with Cypress.Commands.add('byTestID', ...)
returns the cy.get call.
In `@ui-tests-cy/tests/nmstate/nncp.cy.ts`:
- Around line 17-20: The test(s) call createNNCPFromYAML() and visitNNCP() but
lack assertions; update the create/edit/delete YAML flow tests (the test that
calls createNNCPFromYAML() and the similar cases around lines 35-38 and 40-42)
to assert final state: after create, assert the new NNCP row is visible (use the
NNCP name/identifier produced by createNNCPFromYAML() with cy.contains or a
table row selector); after edit, assert the updated field(s) appear in that row;
after delete, assert the NNCP row is not present. If createNNCPFromYAML() does
not return the resource name, modify it to return or expose the created NNCP
identifier so tests can assert against it.
In `@ui-tests-cy/tests/nmstate/vmn.cy.ts`:
- Around line 17-44: The "view Virtual machine network details" test depends on
prior creates; make it independent by ensuring it creates and cleans up its own
fixture: call createVMNetwork(VM_NETWORK_PROJECT_NAME) and navigateToVMN() at
the start of the it('view Virtual machine network details', ...) test, perform
the detail assertions, then call navigateToVMN() and
deleteVMNetwork(VM_NETWORK_PROJECT_NAME) in teardown, or alternatively collapse
the related specs into a single comprehensive test that runs createVMNetwork,
asserts list presence, clicks to view details, then deletes via deleteVMNetwork
— use the existing helpers createVMNetwork, navigateToVMN, and deleteVMNetwork
to implement this.
In `@ui-tests-cy/tsconfig.json`:
- Line 5: Update the TypeScript config so single-file transpilation matches the
esbuild-loader setup: set "isolatedModules": true in ui-tests-cy/tsconfig.json
to align with the esbuild-loader rule for /\.ts$/ used in
ui-tests-cy/plugins/index.ts; this prevents TypeScript constructs that esbuild
can't safely emit (e.g., const enums or certain export-from patterns) from
slipping through and causing runtime/build issues. Ensure no other tsconfig
option contradicts isolatedModules before committing.
In `@ui-tests-cy/views/nncp.ts`:
- Around line 33-41: The final existence check is too broad; update deleteNNCP
to scope the post-delete assertion to the table row or table container so it
only verifies the row for the deleted policy is gone — e.g., after the second
Delete click, change the global cy.contains(name).should('not.exist') to a
scoped check like cy.contains('tr', name).should('not.exist') or
cy.get('<tableSelector>').contains('tr', name).should('not.exist'); keep the
rest of deleteNNCP (actionsBtn usage, confirmation typing) the same.
In `@ui-tests-cy/views/vmNetwork.ts`:
- Around line 52-54: Remove the { force: true } usage on both the
'`#delete-vm-network-modal-acknowledge-checkbox`' click and the "button" that
contains 'Delete'; instead assert actionability and interact normally so Cypress
can surface real UI issues. Specifically, replace forced clicks with checks like
ensuring the modal and elements are visible/enabled (e.g.,
cy.get('`#delete-vm-network-modal`').should('be.visible'),
cy.get('`#delete-vm-network-modal-acknowledge-checkbox`').should('be.visible').check()
or .click(), and
cy.get('button').contains('Delete').should('be.visible').click()) and wait for
any animation/overlay to disappear before clicking the Delete button; remove the
{ force: true } on both selectors so tests fail if the UI is not actually
actionable.
---
Nitpick comments:
In `@research-flakiness.sh`:
- Line 58: The current expression $((FAIL_COUNT * 100 / TOTAL_RUNS)) uses
integer division and truncates percentages; replace it by computing a
floating-point percentage (e.g., using awk or bc with a set scale) and format it
to at least one decimal place, store that result in a variable like
FAILURE_RATE, and then use that variable in the markdown line instead of the
integer expression so the displayed failure rate shows decimal precision.
- Around line 18-20: Before running destructive removal and launching the test
script, add existence and executability checks: verify the directory
"ui-tests-cy/gui-test-screenshots" exists (e.g., test -d) before running rm -rf
on its contents, and verify "./test-cypress.sh" exists and is executable (e.g.,
test -x or test -f and chmod +x) before invoking it; perform the
"./test-cypress.sh" validation once before entering the loop referenced near the
existing loop start (the comment mentioned line 13) and fail-fast with a clear
error message if checks fail.
In `@ui-tests-cy/.eslintrc`:
- Around line 1-15: Add import-sorting to the Cypress ESLint config by
installing eslint-plugin-simple-import-sort as a devDependency in
ui-tests-cy/package.json and then update .eslintrc to include
"plugin:simple-import-sort/recommended" in the "extends" array (and/or add
"simple-import-sort" to "plugins") so that the simple-import-sort rules are
applied to the Cypress tests; ensure the rule set is consistent with the main
project's configuration and don't remove existing rules like "no-console" or
"`@typescript-eslint/no-namespace`".
In `@ui-tests-cy/support/index.ts`:
- Around line 10-18: The custom Cypress.log override suppresses entries for
displayName 'fetch' or 'xhr' but returns undefined, violating the expected
return contract; update the override in the Cypress.log function (the override
referencing origLog and opts.displayName) to return a safe value when
suppressing logs—either return null or a minimal stub log object with the same
shape the caller expects (e.g., id/message properties) instead of returning
undefined so downstream code receives a predictable return value.
In `@ui-tests-cy/support/selectors.ts`:
- Around line 69-77: The local variable name t_o in the Cypress custom commands
for checkTitle and checkSubTitle is abbreviated and harms readability; rename
t_o to a descriptive name like timeoutValue (or timeout) in both functions,
update the assignment (const timeoutValue = timeout ? timeout : MINUTE * 3) and
replace all uses of t_o with timeoutValue so the commands
Cypress.Commands.add('checkTitle', ...) and
Cypress.Commands.add('checkSubTitle', ...) remain functionally identical but
clearer.
In `@ui-tests-cy/tests/nmstate/nns.cy.ts`:
- Line 13: Replace the fixed sleep cy.wait(2 * SECOND) (and the other
occurrences noted) with a condition-based Cypress wait that targets the specific
UI element/state change instead of a timed delay: locate the test steps
surrounding cy.wait(2 * SECOND), identify the DOM selector or text that
indicates the app progressed (e.g., a button enabling/disabling, a status text
change, or an element appearing/disappearing), and replace the sleep with an
assertion like cy.get(<selector>, { timeout: <ms> }).should(<expectedCondition>)
or cy.contains(<text>, { timeout: <ms> }).should(<expectedCondition>) so the
test waits until the real UI state is reached rather than a fixed interval;
apply the same replacement for the other cy.wait occurrences referenced in the
comment.
In `@ui-tests-cy/views/nncp.ts`:
- Around line 19-31: The createNNCPFromYAML helper uses fragile hardcoded waits
and a broad page-text check for duplicate errors; replace cy.wait(3 * SECOND)
and cy.wait(5 * SECOND) with targeted assertions/commands that wait for specific
elements or network calls (e.g., wait for the YAML modal to be visible, wait for
the Create button to become enabled, or intercept the create request and wait on
its route), and replace the body text check with a scoped assertion that looks
for the precise duplicate error element or toast (search within the modal or the
notification container) and then click the Cancel button if that specific error
is present; update references in createNNCPFromYAML and usages of
itemCreateBtn/Create button selectors accordingly.
In `@ui-tests-cy/views/physicalNetwork.ts`:
- Around line 17-20: Remove the redundant hardcoded wait: after confirming the
input is visible with cy.get(physicalNetworkNameInput, { timeout: MINUTE
}).should('be.visible'), delete the cy.wait(2 * SECOND) call and proceed to
clear and type into the element (the subsequent
cy.get(physicalNetworkNameInput).clear() and .type(networkName)); this relies on
Cypress's retry behavior on the visible assertion and avoids unnecessary test
slowdown.
- Around line 36-39: The test jumps from "Step 3" to "Step 5" and uses a
hardcoded cy.wait(3 * SECOND); update the comments to add a "Step 4: Confirm
creation initiated" line and replace the fixed wait with an assertion-based wait
that verifies the post-create state (for example, after clicking the 'Create
network' button use cy.contains('Network created', { timeout: MINUTE
}).should('be.visible') or assert navigation with cy.url().should('include',
'/networks') or another app-specific success indicator); keep the existing
selectors ('Create network' button, MINUTE, SECOND) but remove the static wait
and assert the actual success condition instead.
In `@ui-tests-cy/views/vmNetwork.ts`:
- Around line 20-44: The createVMNetwork function uses hardcoded cy.wait()
calls; replace them with assertions that wait for specific UI states: after
opening the create dialog assert the modal header appears (already present) and
wait for vmnNameInput to be visible before interacting instead of
wait(2*SECOND); after clicking Next assert the next step or a form section
(e.g., project mapping controls or a stepper indicator) is visible instead of
wait(5*SECOND); after clicking Create assert success or error toast/text (e.g.,
look for confirmation message or for the "already exists" text) and if the
"already exists" text is present call navigateToVMN(); remove all cy.wait()
calls and use should('be.visible'), contains(...).should('exist'), or
intercept+wait for network responses as appropriate to ensure deterministic
timing while keeping selectors like itemCreateBtn, vmnNameInput, submitBtn, and
navigateToVMN referenced for locating changes.
- Around line 39-43: The test is scanning the whole page body for the string
"already exists", which can produce false positives; change the check to only
inspect alert elements (e.g., use selectors like [role="alert"], .pf-c-alert or
the same alert selector used in nncp.ts) instead of cy.get('body'), then call
navigateToVMN() only if an alert's text includes "already exists" so the match
is limited to real UI alerts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 08975f2f-7732-47c1-954a-3bd7a6ca6dfb
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonui-tests-cy/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (27)
.env.example.gitignorecleanup.shresearch-flakiness.shsetup.shtest-cypress.shui-tests-cy/.eslintrcui-tests-cy/cypress.config.jsui-tests-cy/package.jsonui-tests-cy/plugins/index.tsui-tests-cy/reporter-config.jsonui-tests-cy/support/commands.tsui-tests-cy/support/index.tsui-tests-cy/support/login.tsui-tests-cy/support/nav.tsui-tests-cy/support/selectors.tsui-tests-cy/tests/all.cy.tsui-tests-cy/tests/nmstate/nncp.cy.tsui-tests-cy/tests/nmstate/nns.cy.tsui-tests-cy/tests/nmstate/vmn.cy.tsui-tests-cy/tests/setup/login.cy.tsui-tests-cy/tsconfig.jsonui-tests-cy/utils/const/base.tsui-tests-cy/views/nncp.tsui-tests-cy/views/physicalNetwork.tsui-tests-cy/views/selector-common.tsui-tests-cy/views/vmNetwork.ts
Comment on lines
+7
to
+12
| TEST_NS="${TEST_NS:-nmstate-test-ns}" | ||
|
|
||
| cleanup () { | ||
| echo "Cleaning up nmstate test resources..." | ||
| oc delete nncp test-form-nncp test-vm-nncp example test-nncp-physical-network test-vm-physical-network --ignore-not-found --wait=true --timeout=180s | ||
| oc delete --ignore-not-found=true virtualmachinenetwork test-vmn-project test-vmn-labeled --wait=true --timeout=60s 2>/dev/null || true |
There was a problem hiding this comment.
Cleanup namespace contract is inconsistent and partially ignored.
setup.sh defaults to cy-test-ns, but this file defaults to nmstate-test-ns, and the virtualmachinenetwork delete command does not target TEST_NS. That can leave leftovers or clean the wrong namespace.
Suggested patch
-TEST_NS="${TEST_NS:-nmstate-test-ns}"
+TEST_NS="${TEST_NS:-cy-test-ns}"
@@
- oc delete --ignore-not-found=true virtualmachinenetwork test-vmn-project test-vmn-labeled --wait=true --timeout=60s 2>/dev/null || true
+ oc delete --ignore-not-found=true virtualmachinenetwork test-vmn-project test-vmn-labeled -n "${TEST_NS}" --wait=true --timeout=60s 2>/dev/null || true🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cleanup.sh` around lines 7 - 12, The cleanup function uses a different
default TEST_NS and omits namespace scoping for the virtualmachinenetwork
delete; change TEST_NS default to match setup.sh's value (cy-test-ns) and ensure
all oc delete invocations in cleanup (including the virtualmachinenetwork delete
and the first oc delete of nncp resources) explicitly target the namespace by
adding -n "$TEST_NS" (or equivalent) so cleanup consistently operates in the
intended namespace; update references to TEST_NS in the cleanup() function
accordingly.
Comment on lines
+13
to
+23
| "@cypress/webpack-preprocessor": "^6.0.2", | ||
| "cypress": "^15.3.0", | ||
| "cypress-multi-reporters": "^2.0.4", | ||
| "dotenv": "^16.4.7", | ||
| "esbuild-loader": "^4.2.2", | ||
| "mocha-junit-reporter": "^2.2.1", | ||
| "mochawesome": "^7.1.3", | ||
| "mochawesome-merge": "^4.3.0", | ||
| "mochawesome-report-generator": "^6.2.0", | ||
| "typescript": "^5.9.3", | ||
| "webpack": "^5.104.1" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
# Verify whether package.json still contains semver ranges in dependencies/devDependencies
jq -r '
.devDependencies // {} | to_entries[] | select(.value|test("^[\\^~]")) | "\(.key)=\(.value)"
' ui-tests-cy/package.jsonRepository: openshift/nmstate-console-plugin
Length of output: 340
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
# Check for semver ranges in dependencies AND devDependencies
jq -r '
( .dependencies // {} ) | to_entries[] | select(.value|test("^[\\^~]")) | "dependencies:\(.key)=\(.value)"
' ui-tests-cy/package.json
jq -r '
( .devDependencies // {} ) | to_entries[] | select(.value|test("^[\\^~]")) | "devDependencies:\(.key)=\(.value)"
' ui-tests-cy/package.jsonRepository: openshift/nmstate-console-plugin
Length of output: 516
Pin exact devDependencies versions in ui-tests-cy/package.json (remove ^ ranges).
ui-tests-cy/package.json devDependencies use caret semver ranges (e.g., cypress, webpack, typescript), which weakens supply-chain reproducibility.
Suggested patch
- "`@cypress/webpack-preprocessor`": "^6.0.2",
- "cypress": "^15.3.0",
- "cypress-multi-reporters": "^2.0.4",
- "dotenv": "^16.4.7",
- "esbuild-loader": "^4.2.2",
- "mocha-junit-reporter": "^2.2.1",
- "mochawesome": "^7.1.3",
- "mochawesome-merge": "^4.3.0",
- "mochawesome-report-generator": "^6.2.0",
- "typescript": "^5.9.3",
- "webpack": "^5.104.1"
+ "`@cypress/webpack-preprocessor`": "6.0.2",
+ "cypress": "15.3.0",
+ "cypress-multi-reporters": "2.0.4",
+ "dotenv": "16.4.7",
+ "esbuild-loader": "4.2.2",
+ "mocha-junit-reporter": "2.2.1",
+ "mochawesome": "7.1.3",
+ "mochawesome-merge": "4.3.0",
+ "mochawesome-report-generator": "6.2.0",
+ "typescript": "5.9.3",
+ "webpack": "5.104.1"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ui-tests-cy/package.json` around lines 13 - 23, The devDependencies in
ui-tests-cy package.json are using caret ranges which reduces reproducibility;
update the dependency entries for "`@cypress/webpack-preprocessor`", "cypress",
"cypress-multi-reporters", "dotenv", "esbuild-loader", "mocha-junit-reporter",
"mochawesome", "mochawesome-merge", "mochawesome-report-generator",
"typescript", and "webpack" to pin exact versions by removing the leading '^'
from each version string so the package.json lists exact versions.
Source: Coding guidelines
| config.baseUrl = `${process.env.BRIDGE_BASE_ADDRESS || 'http://localhost:9000/'}`; | ||
| config.env.BRIDGE_KUBEADMIN_PASSWORD = process.env.BRIDGE_KUBEADMIN_PASSWORD; | ||
| config.env.TEST_NS = process.env.TEST_NS; | ||
| config.env.HIDE_XHR = process.env.HIDE_XHR; |
There was a problem hiding this comment.
Normalize HIDE_XHR to boolean before injecting into Cypress env.
Passing raw string env values means "false" is treated as truthy, which can incorrectly enable log hiding.
Suggested patch
- config.env.HIDE_XHR = process.env.HIDE_XHR;
+ config.env.HIDE_XHR = String(process.env.HIDE_XHR).toLowerCase() === 'true';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| config.env.HIDE_XHR = process.env.HIDE_XHR; | |
| config.env.HIDE_XHR = String(process.env.HIDE_XHR).toLowerCase() === 'true'; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ui-tests-cy/plugins/index.ts` at line 28, The code sets config.env.HIDE_XHR
to the raw process.env.HIDE_XHR string which treats "false" as truthy; update
the assignment in plugins/index.ts so that the value is normalized to a boolean
(e.g., convert process.env.HIDE_XHR to true only when it equals the string
"true", otherwise false, and default to false if undefined) before assigning to
config.env.HIDE_XHR so Cypress receives a real boolean.
Comment on lines
+17
to
+20
| it('create NNCP with YAML', () => { | ||
| createNNCPFromYAML(); | ||
| cy.visitNNCP(); | ||
| }); |
There was a problem hiding this comment.
Add outcome assertions for YAML create/edit/delete flows.
These cases currently perform actions without asserting final state, so they can pass even when behavior regresses. Please assert success criteria (e.g., created/updated row visible, deleted row absent) after each operation.
Also applies to: 35-38, 40-42
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ui-tests-cy/tests/nmstate/nncp.cy.ts` around lines 17 - 20, The test(s) call
createNNCPFromYAML() and visitNNCP() but lack assertions; update the
create/edit/delete YAML flow tests (the test that calls createNNCPFromYAML() and
the similar cases around lines 35-38 and 40-42) to assert final state: after
create, assert the new NNCP row is visible (use the NNCP name/identifier
produced by createNNCPFromYAML() with cy.contains or a table row selector);
after edit, assert the updated field(s) appear in that row; after delete, assert
the NNCP row is not present. If createNNCPFromYAML() does not return the
resource name, modify it to return or expose the created NNCP identifier so
tests can assert against it.
Comment on lines
+17
to
+44
| it('create prerequisites from warning page', () => { | ||
| attemptToCreateVMNetwork(); | ||
| }); | ||
|
|
||
| it('create Virtual machine network', () => { | ||
| createVMNetwork(VM_NETWORK_PROJECT_NAME); | ||
| navigateToVMN(); | ||
| cy.contains(VM_NETWORK_PROJECT_NAME, { timeout: MINUTE }).should('exist'); | ||
| }); | ||
|
|
||
| it('create second Virtual machine network', () => { | ||
| createVMNetwork(VM_NETWORK_LABELED_NAME); | ||
| navigateToVMN(); | ||
| cy.contains(VM_NETWORK_LABELED_NAME, { timeout: MINUTE }).should('exist'); | ||
| }); | ||
|
|
||
| it('view Virtual machine network details', () => { | ||
| cy.contains('a', VM_NETWORK_PROJECT_NAME).click(); | ||
| cy.contains('h1', VM_NETWORK_PROJECT_NAME, { timeout: MINUTE }).should('exist'); | ||
| cy.contains('Virtual machine network details').should('be.visible'); | ||
| cy.contains('Connected projects').should('be.visible'); | ||
| navigateToVMN(); | ||
| }); | ||
|
|
||
| it('delete Virtual machine networks', () => { | ||
| deleteVMNetwork(VM_NETWORK_PROJECT_NAME); | ||
| deleteVMNetwork(VM_NETWORK_LABELED_NAME); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Tests lack independence - failures will cascade.
The test at lines 33-39 ("view Virtual machine network details") depends on the earlier tests (lines 21-31) having successfully created VM_NETWORK_PROJECT_NAME. If test creation fails or is skipped, the details test will also fail. This violates the Cypress best practice that each test should be independently runnable.
Consider restructuring to ensure each test sets up its own required state, or use a single comprehensive test for the full workflow.
💡 Example restructuring approach
Option 1: Single comprehensive test
it('complete VM network lifecycle', () => {
// Create, verify, view details, delete all in one test
createVMNetwork(VM_NETWORK_PROJECT_NAME);
navigateToVMN();
cy.contains(VM_NETWORK_PROJECT_NAME, { timeout: MINUTE }).should('exist');
// View details
cy.contains('a', VM_NETWORK_PROJECT_NAME).click();
cy.contains('h1', VM_NETWORK_PROJECT_NAME, { timeout: MINUTE }).should('exist');
// ... rest of assertions
// Cleanup
navigateToVMN();
deleteVMNetwork(VM_NETWORK_PROJECT_NAME);
});Option 2: Independent tests with setup
it('view Virtual machine network details', () => {
// Ensure prerequisite exists
createVMNetwork(VM_NETWORK_PROJECT_NAME);
navigateToVMN();
// Now test details view
cy.contains('a', VM_NETWORK_PROJECT_NAME).click();
// ... assertions
// Cleanup this test's resources
navigateToVMN();
deleteVMNetwork(VM_NETWORK_PROJECT_NAME);
});🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ui-tests-cy/tests/nmstate/vmn.cy.ts` around lines 17 - 44, The "view Virtual
machine network details" test depends on prior creates; make it independent by
ensuring it creates and cleans up its own fixture: call
createVMNetwork(VM_NETWORK_PROJECT_NAME) and navigateToVMN() at the start of the
it('view Virtual machine network details', ...) test, perform the detail
assertions, then call navigateToVMN() and
deleteVMNetwork(VM_NETWORK_PROJECT_NAME) in teardown, or alternatively collapse
the related specs into a single comprehensive test that runs createVMNetwork,
asserts list presence, clicks to view details, then deletes via deleteVMNetwork
— use the existing helpers createVMNetwork, navigateToVMN, and deleteVMNetwork
to implement this.
| "compilerOptions": { | ||
| "noEmit": true, | ||
| "types": ["cypress", "node"], | ||
| "isolatedModules": false, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check esbuild and isolated modules documentation/requirements
# Search for any existing TypeScript files in the Cypress tests that might rely on non-isolated module patterns
rg -n -C3 'export.*from' ui-tests-cy/ --type=ts
# Check if there are any const enums (which require isolatedModules: false)
rg -n 'const enum' ui-tests-cy/ --type=tsRepository: openshift/nmstate-console-plugin
Length of output: 58
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== ui-tests-cy/tsconfig.json =="
if [ -f ui-tests-cy/tsconfig.json ]; then
cat -n ui-tests-cy/tsconfig.json | head -n 120
else
echo "Missing ui-tests-cy/tsconfig.json"
fi
echo
echo "== ui-tests-cy/plugins/index.ts (lines 1-120) =="
if [ -f ui-tests-cy/plugins/index.ts ]; then
cat -n ui-tests-cy/plugins/index.ts | head -n 160
else
echo "Missing ui-tests-cy/plugins/index.ts"
fi
echo
echo "== Search: isolatedModules usage across repo =="
rg -n --hidden --glob '!*node_modules/*' "isolatedModules" .
echo
echo "== Search: export ... from (non-type-only) =="
rg -n --type=ts '^\s*export\s+\{[^}]+\}\s+from\s+["'\'']' ui-tests-cy/ || true
echo
echo "== Search: export * from (non-type-only) =="
rg -n --type=ts '^\s*export\s+\*\s+from\s+["'\'']' ui-tests-cy/ || true
echo
echo "== Search: import { ... } from (non-type-only) =="
rg -n --type=ts '^\s*import\s+\{[^}]+\}\s+from\s+["'\'']' ui-tests-cy/ || true
echo
echo "== Search: import type { ... } from =="
rg -n --type=ts '^\s*import\s+type\s+\{[^}]+\}\s+from\s+["'\'']' ui-tests-cy/ || true
echo
echo "== Search: const enum in ui-tests-cy =="
rg -n --type=ts 'const enum' ui-tests-cy/ || trueRepository: openshift/nmstate-console-plugin
Length of output: 3823
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Search: export { ... } from =="
rg -n --type=ts '^\s*export\s*\{\s*[^}]+\s*\}\s*from\s*["'\''']' ui-tests-cy/ || true
echo
echo "== Search: export type { ... } from =="
rg -n --type=ts '^\s*export\s+type\s*\{\s*[^}]+\s*\}\s*from\s*["'\''']' ui-tests-cy/ || true
echo
echo "== Search: export { type ... } from =="
rg -n --type=ts '^\s*export\s*\{\s*type\s+[^}]+\}\s*from\s*["'\''']' ui-tests-cy/ || true
echo
echo "== Search: export * from (again, but scoped) =="
rg -n --type=ts '^\s*export\s*\*\s*from\s*["'\''']' ui-tests-cy/ || trueRepository: openshift/nmstate-console-plugin
Length of output: 198
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Search: export { ... } from =="
rg -n --type=ts '^\s*export\s*\{\s*[^}]+\s*\}\s*from\s*["'\'']' ui-tests-cy/ || true
echo
echo "== Search: export type { ... } from =="
rg -n --type=ts '^\s*export\s+type\s*\{\s*[^}]+\s*\}\s*from\s*["'\'']' ui-tests-cy/ || true
echo
echo "== Search: export { type ... } from =="
rg -n --type=ts '^\s*export\s*\{\s*type\s+[^}]+\}\s*from\s*["'\'']' ui-tests-cy/ || true
echo
echo "== Search: export * from =="
rg -n --type=ts '^\s*export\s*\*\s*from\s*["'\'']' ui-tests-cy/ || true
echo
echo "== Search: type-only imports =="
rg -n --type=ts '^\s*import\s+type\s+\{[^}]+\}\s+from\s*["'\'']' ui-tests-cy/ || trueRepository: openshift/nmstate-console-plugin
Length of output: 252
🌐 Web query:
esbuild-loader isolatedModules true requirement
💡 Result:
In esbuild-loader, it is required to set isolatedModules to true in your tsconfig.json to prevent mis-compilation errors [1][2]. Because esbuild is a single-file transpiler, it does not have knowledge of the full TypeScript type system or external file dependencies when processing an individual file [3]. Setting isolatedModules: true instructs the TypeScript compiler to warn you if you write code that cannot be safely transpiled in isolation—such as re-exporting types or certain uses of namespaces and ambient const enums [1][3]. Without this setting, these features could lead to runtime errors because the transpiler may be unable to correctly replace or remove code that requires type-level information [3].
Citations:
- 1: https://www.npmjs.com/package/esbuild-loader
- 2: https://registry.npmjs.org/esbuild-loader
- 3: https://www.typescriptlang.org/tsconfig/isolatedModules.html
🌐 Web query:
esbuild const enum isolatedModules
💡 Result:
esbuild fully supports const enums by inlining their values during compilation [1][2][3], provided the enum is not ambient (i.e., not declared with the declare keyword) [4][2][3]. Because esbuild operates by transpiling each file in isolation, it lacks the full type-checking context required to resolve ambient const enum declarations that exist only in type definitions [5][3][6]. Consequently, referencing ambient const enum members results in a runtime error because the enum object is not emitted, and the reference cannot be inlined [4][5][3]. To ensure your project remains safe and avoids these runtime errors, it is highly recommended to enable the isolatedModules: true option in your tsconfig.json [5][6][7]. When this flag is set, the TypeScript compiler will detect and error on unsafe patterns—such as referencing ambient const enum members—before you even run your build [5][2][3]. Key takeaways: * Standard const enum usage is supported and will be inlined correctly [4][2][3]. * Ambient const enum (i.e., declare const enum) is not supported, as it cannot be inlined by a single-file transpiler [4][3]. You should remove the declare modifier if you want to use it [4][3]. * Always enable isolatedModules: true when using esbuild to prevent accidental usage of TypeScript features that require multi-file type analysis [3][6][7].
Citations:
- 1: Inline and erase local const enums evanw/esbuild#534
- 2: [engineering] add build/checker for typescript
isolatedModules: truemicrosoft/vscode#252605 - 3: [Feature] Inline const enum values in TypeScript evanw/esbuild#128
- 4: declare const enum values not inlined - ReferenceError at runtime evanw/esbuild#4394
- 5: https://www.typescriptlang.org/tsconfig/isolatedModules.html
- 6: https://esbuild.github.io/content-types/
- 7: How to properly import enums evanw/esbuild#2298
🌐 Web query:
site:npmjs.com/package/esbuild-loader isolatedModules true
💡 Result:
The isolatedModules option is a TypeScript compiler setting that ensures each file can be safely transpiled without relying on other files for type information. When set to true, TypeScript will issue a warning or error if it encounters code that cannot be processed in isolation [1]. Key aspects of isolatedModules: 1. Transpilation Compatibility: It is essential when using transpilers that perform single-file compilation, such as esbuild, Babel, or SWC [1]. These tools process files individually and do not have access to the full type-checking context of a project. 2. Unsupported Features: When isolatedModules is enabled, TypeScript prevents the use of certain language features that require cross-file knowledge or complex type interpretation to compile correctly. For example, it helps prevent mis-compilation issues related to re-exporting types, which might not exist in the emitted JavaScript but could cause issues in tools that expect pure single-file processing [1]. 3. Recommended Usage: It is highly recommended to set isolatedModules to true if you are using modern build pipelines or bundlers that rely on transpilers other than the standard TypeScript compiler (tsc) for emission [1]. This ensures that your code remains compatible with the constraints of these tools. While this setting enforces stricter rules on how you write code, it does not change the core type-checking behavior of the TypeScript compiler itself; rather, it makes your codebase more portable and robust across different build environments.
Citations:
🌐 Web query:
site:github.com/evanw/esbuild isolatedModules
💡 Result:
The isolatedModules flag in TypeScript is a configuration option in tsconfig.json that instructs the TypeScript compiler to ensure that every file can be safely transpiled in isolation, without relying on type information from other files [1][2]. When isolatedModules is enabled (set to true), TypeScript will flag code patterns that cannot be safely transpiled if the compiler only looks at the single file currently being processed [3][2]. This is critical for build tools that process files one by one—such as esbuild, Babel, or the TypeScript transpileModule API—as these tools cannot perform cross-file type analysis to determine if an imported symbol is a value or a type [3][4][2]. Key implications of enabling isolatedModules include: - Prevents problematic re-exports: It forces the use of export type {T} from '...' instead of export {T} from '...', ensuring that type-only exports are explicitly marked so the compiler knows they can be safely removed during transpilation [3][1]. - Ensures safe transpilation: It guarantees that the generated JavaScript will work correctly even if the transpiler lacks knowledge of the broader project structure or type definitions [4][2]. - Standardizes behavior: By enforcing these constraints at compile time, it ensures that your TypeScript code is compatible with transpilers that operate independently of full type-checking [5][2]. It is highly recommended to enable isolatedModules when using modern, fast build tools like esbuild, Vite, or others that rely on single-file transpilation, as it helps prevent runtime errors caused by missing imports or incorrectly preserved types [3][6][2].
Citations:
- 1: TypeScript parsing bug - cascading issue with exporting of nihilated TS interface evanw/esbuild#314
- 2: bug: run-time error when enum is declared evanw/esbuild#890
- 3: Can not erasure types evanw/esbuild#863
- 4: No matching export for types exported via
export { ... }evanw/esbuild#1941 - 5: Error when importing an exported namespace evanw/esbuild#1550
- 6: How exclude type which export by third library inside output? evanw/esbuild#341
Set isolatedModules: true for Cypress esbuild-loader TypeScript transpilation.
ui-tests-cy/plugins/index.ts configures esbuild-loader for \.ts$, but ui-tests-cy/tsconfig.json sets isolatedModules: false (line 5). Single-file transpilation benefits from isolatedModules: true to avoid TS patterns that esbuild may not emit safely; the current Cypress tests don’t contain obvious problematic export ... from/const enum usage, but the mismatch can still cause future build/runtime issues.
🔧 Proposed fix
{
"compilerOptions": {
"noEmit": true,
"types": ["cypress", "node"],
- "isolatedModules": false,
+ "isolatedModules": true,
"module": "esnext",
"target": "es2016",
"moduleResolution": "node",
"esModuleInterop": true,
"strict": false
},
"include": ["node_modules/cypress", "./**/*.ts"]
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| "isolatedModules": false, | |
| { | |
| "compilerOptions": { | |
| "noEmit": true, | |
| "types": ["cypress", "node"], | |
| "isolatedModules": true, | |
| "module": "esnext", | |
| "target": "es2016", | |
| "moduleResolution": "node", | |
| "esModuleInterop": true, | |
| "strict": false | |
| }, | |
| "include": ["node_modules/cypress", "./**/*.ts"] | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ui-tests-cy/tsconfig.json` at line 5, Update the TypeScript config so
single-file transpilation matches the esbuild-loader setup: set
"isolatedModules": true in ui-tests-cy/tsconfig.json to align with the
esbuild-loader rule for /\.ts$/ used in ui-tests-cy/plugins/index.ts; this
prevents TypeScript constructs that esbuild can't safely emit (e.g., const enums
or certain export-from patterns) from slipping through and causing runtime/build
issues. Ensure no other tsconfig option contradicts isolatedModules before
committing.
Comment on lines
+33
to
+41
| export const deleteNNCP = (name: string) => { | ||
| cy.contains('tr', name).within(() => { | ||
| cy.get(actionsBtn).first().click(); | ||
| }); | ||
| cy.contains('button', 'Delete').click(); | ||
| cy.get('#text-confirmation').type(name); | ||
| cy.contains('button', 'Delete').click(); | ||
| cy.contains(name).should('not.exist'); | ||
| }; |
There was a problem hiding this comment.
Scope the deletion verification assertion.
Line 40's assertion cy.contains(name).should('not.exist') will match the policy name anywhere on the page (breadcrumbs, navigation, headers). This could pass even if the resource still exists in the table.
🐛 Proposed fix to scope the assertion
cy.contains('tr', name).within(() => {
cy.get(actionsBtn).first().click();
});
cy.contains('button', 'Delete').click();
cy.get('`#text-confirmation`').type(name);
cy.contains('button', 'Delete').click();
- cy.contains(name).should('not.exist');
+ // Verify the row is removed from the table
+ cy.contains('tr', name).should('not.exist');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ui-tests-cy/views/nncp.ts` around lines 33 - 41, The final existence check is
too broad; update deleteNNCP to scope the post-delete assertion to the table row
or table container so it only verifies the row for the deleted policy is gone —
e.g., after the second Delete click, change the global
cy.contains(name).should('not.exist') to a scoped check like cy.contains('tr',
name).should('not.exist') or cy.get('<tableSelector>').contains('tr',
name).should('not.exist'); keep the rest of deleteNNCP (actionsBtn usage,
confirmation typing) the same.
Comment on lines
+52
to
+54
| cy.get('#delete-vm-network-modal-acknowledge-checkbox').click({ force: true }); | ||
| cy.wait(SECOND); | ||
| cy.get('button').contains('Delete').last().click({ force: true }); |
There was a problem hiding this comment.
Remove { force: true } - it bypasses Cypress safety checks.
Lines 52 and 54 use { force: true } to click the checkbox and delete button. This bypasses Cypress's actionability checks (visibility, enabled state, not covered by other elements) and can hide real UI issues such as overlapping modals, disabled buttons, or elements not actually visible to users.
🔒 Recommended fix
- cy.get('`#delete-vm-network-modal-acknowledge-checkbox`').click({ force: true });
+ cy.get('`#delete-vm-network-modal-acknowledge-checkbox`').should('be.visible').click();
cy.wait(SECOND);
- cy.get('button').contains('Delete').last().click({ force: true });
+ cy.get('button').contains('Delete').last().should('be.enabled').click();If the elements are genuinely not actionable without force, that indicates a real UI issue that should be fixed rather than bypassed in tests.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| cy.get('#delete-vm-network-modal-acknowledge-checkbox').click({ force: true }); | |
| cy.wait(SECOND); | |
| cy.get('button').contains('Delete').last().click({ force: true }); | |
| cy.get('`#delete-vm-network-modal-acknowledge-checkbox`').should('be.visible').click(); | |
| cy.wait(SECOND); | |
| cy.get('button').contains('Delete').last().should('be.enabled').click(); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ui-tests-cy/views/vmNetwork.ts` around lines 52 - 54, Remove the { force:
true } usage on both the '`#delete-vm-network-modal-acknowledge-checkbox`' click
and the "button" that contains 'Delete'; instead assert actionability and
interact normally so Cypress can surface real UI issues. Specifically, replace
forced clicks with checks like ensuring the modal and elements are
visible/enabled (e.g., cy.get('`#delete-vm-network-modal`').should('be.visible'),
cy.get('`#delete-vm-network-modal-acknowledge-checkbox`').should('be.visible').check()
or .click(), and
cy.get('button').contains('Delete').should('be.visible').click()) and wait for
any animation/overlay to disappear before clicking the Delete button; remove the
{ force: true } on both selectors so tests fail if the UI is not actually
actionable.
lkladnit
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (6)
ui-tests-cy/support/selectors.ts (1)
29-34:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winMissing return statement breaks Cypress command chaining.
The
byTestIDcommand does not return the result ofcy.get(...), breaking Cypress's chainable API. All subsequent.click(),.should(), etc., chained tocy.byTestID(...)will fail.🐛 Proposed fix
Cypress.Commands.add( 'byTestID', (selector: string, options?: Partial<Loggable & Shadow & Timeoutable & Withinable>) => { - cy.get(`[data-test="${selector}"]`, options); + return cy.get(`[data-test="${selector}"]`, options); }, );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui-tests-cy/support/selectors.ts` around lines 29 - 34, The custom Cypress command 'byTestID' currently calls cy.get(`[data-test="${selector}"]`, options) but does not return it, which breaks Cypress chaining; update the command implementation for 'byTestID' (the Cypress.Commands.add call) to return the result of cy.get(...) so callers can chain .click(), .should(), etc. normally.setup.sh (1)
7-11:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix namespace defaulting; current expansion is incorrect.
Line 7 uses substring expansion
${TEST_NS:cy-test-ns}instead of the fallback default operator${TEST_NS:-cy-test-ns}. This will fail whenTEST_NSis unset. Line 11 also needs quotes around${TEST_NS}to prevent globbing and word splitting.🐛 Proposed fix
-TEST_NS="${TEST_NS:cy-test-ns}" +TEST_NS="${TEST_NS:-cy-test-ns}" @@ - oc get namespace ${TEST_NS} 2>/dev/null || oc create namespace ${TEST_NS} + oc get namespace "${TEST_NS}" 2>/dev/null || oc create namespace "${TEST_NS}"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@setup.sh` around lines 7 - 11, The TEST_NS default expansion is wrong and the variable is unquoted; change the parameter expansion to use the fallback operator and quote usages: replace occurrences of TEST_NS definition `${TEST_NS:cy-test-ns}` with `${TEST_NS:-cy-test-ns}` and wrap variable references in the setup function (the oc get namespace and oc create namespace calls inside the setup() function) with double quotes around `${TEST_NS}` to prevent globbing/word-splitting.cleanup.sh (1)
7-12:⚠️ Potential issue | 🟠 Major | ⚖️ Poor tradeoffCleanup namespace contract is inconsistent and partially ignored.
setup.shdefaults tocy-test-ns, but this file defaults tonmstate-test-ns. Theoc deletecommands on Lines 11-12 do not specify a namespace with-n "${TEST_NS}", which can leave leftovers in the intended namespace or clean the wrong scope.🛠️ Proposed fix
-TEST_NS="${TEST_NS:-nmstate-test-ns}" +TEST_NS="${TEST_NS:-cy-test-ns}" @@ - oc delete nncp test-form-nncp test-vm-nncp example test-nncp-physical-network test-vm-physical-network --ignore-not-found --wait=true --timeout=180s - oc delete --ignore-not-found=true virtualmachinenetwork test-vmn-project test-vmn-labeled --wait=true --timeout=60s 2>/dev/null || true + oc delete -n "${TEST_NS}" nncp test-form-nncp test-vm-nncp example test-nncp-physical-network test-vm-physical-network --ignore-not-found --wait=true --timeout=180s + oc delete -n "${TEST_NS}" --ignore-not-found=true virtualmachinenetwork test-vmn-project test-vmn-labeled --wait=true --timeout=60s 2>/dev/null || true🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cleanup.sh` around lines 7 - 12, The cleanup function uses a different default TEST_NS and omits namespace scope when deleting resources; change the TEST_NS default to match setup.sh (use TEST_NS="${TEST_NS:-cy-test-ns}") and update the oc delete invocations inside cleanup (the commands in function cleanup and the second oc delete line) to include -n "${TEST_NS}" so deletes target the intended namespace; keep --ignore-not-found and timeouts as-is and preserve the 2>/dev/null || true behavior if desired.test-cypress.sh (1)
3-40:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPropagate failures reliably from setup and test execution.
With
set +eon Line 4 and no finalexit "${test_exit_code}", this script will exit 0 even when Cypress fails. This can silently green-light broken E2E runs in CI.🔧 Proposed fix
-set -x -set +e +set -xeuo pipefail @@ - node --max-old-space-size=4096 ./node_modules/.bin/cypress run --env openshift=true --browser ${BRIDGE_E2E_BROWSER_NAME:=electron} --spec "$spec" | tee ./gui-test-screenshots/build.log + node --max-old-space-size=4096 ./node_modules/.bin/cypress run --env openshift=true --browser "${BRIDGE_E2E_BROWSER_NAME:=electron}" --spec "$spec" | tee ./gui-test-screenshots/build.log test_exit_code=${PIPESTATUS[0]} @@ if [ ${test_exit_code} -eq 0 ]; then cleanup fi + exit "${test_exit_code}" fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test-cypress.sh` around lines 3 - 40, The script currently uses set +e so failures from setup() or the Cypress run can be ignored; ensure failures are propagated by enabling strict exit handling and exiting with the Cypress status: enable errexit (replace set +e with set -e or add set -euo pipefail) and after running Cypress use the captured test_exit_code to exit the script (add a final exit "${test_exit_code}" so CI sees failures); also check the return value of setup and cleanup calls (call setup and immediately exit on non-zero) to ensure setup failures don't get swallowed.ui-tests-cy/support/commands.ts (1)
12-24:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCommand injection vulnerability via string interpolation.
The
deleteResourcecommand constructs shell commands by directly interpolatingkind,name, andnamespaceintocy.execcalls. This violates the coding guideline: "Command: no shell=True, os.system, or backtick exec with user input." Although parameters come from test code, the pattern is unsafe. As per coding guidelines, command injection prevention requires avoiding shell execution with interpolated variables.🛡️ Proposed fix using input validation
Cypress.Commands.add('deleteResource', (kind: string, name: string, namespace?: string) => { + // Validate inputs contain only alphanumeric, dash, and underscore + const safePattern = /^[a-zA-Z0-9_-]+$/; + if (!safePattern.test(kind) || !safePattern.test(name)) { + throw new Error('Invalid resource kind or name'); + } + if (namespace && !safePattern.test(namespace)) { + throw new Error('Invalid namespace'); + } + if (!namespace) { cy.exec(`oc delete --ignore-not-found=true ${kind} ${name} --wait=true --timeout=300s`, { failOnNonZeroExit: false, timeout: 5 * MINUTE, }); return; } cy.exec( `oc delete --ignore-not-found=true -n ${namespace} ${kind} ${name} --wait=true --timeout=300s`, { failOnNonZeroExit: false, timeout: 5 * MINUTE }, ); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui-tests-cy/support/commands.ts` around lines 12 - 24, The deleteResource command is vulnerable because it interpolates user-controlled strings into a shell command; fix it by validating and sanitizing the three parameters in the deleteResource function (kind, name, namespace) before calling cy.exec: enforce strict allowlists/regexes (e.g., only letters, numbers, '-', '.', and length limits) or map known resource kinds to an enum, reject anything that doesn't match, and only then build the command; update the cy.exec calls in deleteResource to use those validated values (or fail fast) so no unvalidated input is ever interpolated into the shell string.Source: Coding guidelines
ui-tests-cy/package.json (1)
13-23:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPin exact
devDependenciesversions (remove^ranges).All devDependencies use caret semver ranges, which weakens supply-chain reproducibility. As per coding guidelines, exact versions should be pinned.
🔒 Proposed fix
- "`@cypress/webpack-preprocessor`": "^6.0.2", - "cypress": "^15.3.0", - "cypress-multi-reporters": "^2.0.4", - "dotenv": "^16.4.7", - "esbuild-loader": "^4.2.2", - "mocha-junit-reporter": "^2.2.1", - "mochawesome": "^7.1.3", - "mochawesome-merge": "^4.3.0", - "mochawesome-report-generator": "^6.2.0", - "typescript": "^5.9.3", - "webpack": "^5.104.1" + "`@cypress/webpack-preprocessor`": "6.0.2", + "cypress": "15.3.0", + "cypress-multi-reporters": "2.0.4", + "dotenv": "16.4.7", + "esbuild-loader": "4.2.2", + "mocha-junit-reporter": "2.2.1", + "mochawesome": "7.1.3", + "mochawesome-merge": "4.3.0", + "mochawesome-report-generator": "6.2.0", + "typescript": "5.9.3", + "webpack": "5.104.1"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui-tests-cy/package.json` around lines 13 - 23, The devDependencies in package.json currently use caret ranges (e.g., "`@cypress/webpack-preprocessor`", "cypress", "cypress-multi-reporters", "dotenv", "esbuild-loader", "mocha-junit-reporter", "mochawesome", "mochawesome-merge", "mochawesome-report-generator", "typescript", "webpack"); change each version string to an exact pinned version (remove the leading '^' so entries become e.g. "cypress": "15.3.0") and then regenerate the lockfile by running your package manager install command to persist the pinned versions.Source: Coding guidelines
🧹 Nitpick comments (5)
ui-tests-cy/tests/nmstate/physical.cy.ts (3)
4-7: ⚡ Quick winAdd an
afterhook to clean up created resources.The
beforehook deletes any existing PhysicalNetwork, but there is noafterhook to clean up resources created during the test suite. If tests fail or are interrupted, the test resource will persist in the cluster.♻️ Suggested cleanup hook
adminOnlyDescribe('Test Physical networks', () => { before(() => { cy.deleteResource(K8S_KIND.NNCP, PHYSICAL_NETWORK_TEST_NAME); }); + + after(() => { + cy.deleteResource(K8S_KIND.NNCP, PHYSICAL_NETWORK_TEST_NAME); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui-tests-cy/tests/nmstate/physical.cy.ts` around lines 4 - 7, Add an after hook to ensure created PhysicalNetwork resources are removed after the suite: in the adminOnlyDescribe block that contains the before hook, add an after(() => { ... }) that calls cy.deleteResource(K8S_KIND.NNCP, PHYSICAL_NETWORK_TEST_NAME) (matching the before) so the resource is cleaned up even if tests fail or are interrupted; place this after hook alongside the existing before to guarantee teardown of PHYSICAL_NETWORK_TEST_NAME.
14-17: ⚡ Quick winTest depends on previous test execution.
This test assumes the Physical network already exists from the previous
'create Physical network'test. If this test runs in isolation (e.g., via.only), it will fail. Cypress tests should be independent and runnable individually.Consider adding a
beforeEachhook that ensures the resource exists, or restructure the tests to use nesteddescribeblocks with appropriate setup/teardown.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui-tests-cy/tests/nmstate/physical.cy.ts` around lines 14 - 17, The test 'verify Physical network in list' depends on a prior test creating the network; make it independent by adding setup that ensures the resource exists before running: add a beforeEach (or move into a nested describe with setup) that calls the creation helper used by the 'create Physical network' flow (or an API utility) to create PHYSICAL_NETWORK_TEST_NAME, then call cy.visitPhysicalNetworks() and assert presence; reference the test name 'verify Physical network in list', the helper cy.visitPhysicalNetworks(), and the constant PHYSICAL_NETWORK_TEST_NAME when locating where to add the setup/creation logic.
21-26: ⚡ Quick winReplace hard-coded wait with assertion-based waiting.
Line 26 uses
cy.wait(2 * SECOND), which is a hard-coded time-based wait. This is a Cypress anti-pattern that adds unnecessary delay and can still be flaky if the DOM takes longer than 2 seconds to update.Instead, wait for a specific DOM condition that indicates the row has expanded (e.g., presence of expanded content, an
aria-expanded="true"attribute, or a specific class).♻️ Suggested assertion-based wait
cy.contains('td', PHYSICAL_NETWORK_TEST_NAME) .closest('tbody') .find('td:first-child button') .first() .click(); - cy.wait(2 * SECOND); // Verify the NNCP name appears in the expanded content cy.contains('td', PHYSICAL_NETWORK_TEST_NAME) .closest('tbody') - .contains(PHYSICAL_NETWORK_TEST_NAME) + .contains(PHYSICAL_NETWORK_TEST_NAME, { timeout: MINUTE }) .should('be.visible');By adding a timeout to the assertion itself, Cypress will retry until the condition is met or the timeout expires.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui-tests-cy/tests/nmstate/physical.cy.ts` around lines 21 - 26, Replace the hard-coded cy.wait(2 * SECOND) after clicking the row action so the test waits for a real DOM change: after clicking the button found via cy.contains('td', PHYSICAL_NETWORK_TEST_NAME).closest('tbody').find('td:first-child button').first().click(), assert the expected expanded state (for example, that the clicked button or its container has aria-expanded="true" or that a known expanded-content element/class appears) with an explicit timeout so Cypress will retry until the row is expanded instead of sleeping.ui-tests-cy/tests/nmstate/visit-pages.cy.ts (2)
14-17: 💤 Low valueInconsistent assertion pattern.
This test uses URL assertion (
cy.url().should('include', ...)) while the other navigation tests assert on the presence of anh1element. This inconsistency may be intentional if the Node network configuration page lacks an h1 heading, but it's worth verifying.For consistency and robustness, consider using the same assertion pattern (h1 check) unless there's a specific reason the page structure differs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui-tests-cy/tests/nmstate/visit-pages.cy.ts` around lines 14 - 17, The test "should navigate to Node network configuration" uses cy.clickNavLink(['Networking', 'Node network configuration']) and then asserts via cy.url(), which is inconsistent with other navigation tests that assert the presence of an h1; update the test to assert the page heading instead (e.g., check that the page contains an h1 with the expected text) by replacing or augmenting the cy.url().should(...) assertion with an h1 existence/text assertion targeting the Node network configuration page; if the page truly lacks an h1, add a brief inline comment in this test explaining why URL assertion is kept and consider adding a more specific DOM selector to assert page load consistency.
19-28: ⚡ Quick winConditional test based on DOM state may be flaky.
This test uses
.then()to conditionally execute based on whether'Virtualization'appears in the sidebar text. This pattern can be flaky if the sidebar hasn't fully loaded when the check executes.Consider using a Cypress environment variable or feature flag to determine whether Virtualization is installed, rather than inspecting DOM content. Alternatively, add an explicit wait or assertion to ensure the sidebar is fully loaded before checking its text.
♻️ Alternative: use environment variable
it('should navigate to Virtual machine networks (if Virtualization present)', () => { - cy.get('`#page-sidebar`').then(($sidebar) => { - if ($sidebar.text().includes('Virtualization')) { + if (Cypress.env('HAS_VIRTUALIZATION')) { cy.clickNavLink(['Virtualization', 'Virtual machine networks']); cy.contains('h1', 'Virtual machine networks', { timeout: MINUTE }).should('exist'); - } else { + } else { cy.log('Virtualization section not present — skipping'); - } - }); + } });This would require setting
HAS_VIRTUALIZATIONin your Cypress config or via environment variable, but provides more reliable conditional logic.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui-tests-cy/tests/nmstate/visit-pages.cy.ts` around lines 19 - 28, The conditional DOM check in the test "should navigate to Virtual machine networks (if Virtualization present)" using cy.get('`#page-sidebar`').then(...) is flaky; either read a Cypress env var like HAS_VIRTUALIZATION (Cypress.env('HAS_VIRTUALIZATION')) at the top of the test and branch to call cy.clickNavLink(['Virtualization', 'Virtual machine networks']) and the subsequent cy.contains('h1', 'Virtual machine networks', { timeout: MINUTE }) when true, or if you must detect via DOM, replace the immediate .then() check with a robust wait/assertion (e.g., cy.get('`#page-sidebar`').should('exist').and('not.be.empty') before checking .invoke('text') or use cy.contains('Virtualization', { timeout: MINUTE }).then(...) ) so the sidebar is fully loaded before deciding to navigate or skip.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ui-tests-cy/support/selectors.ts`:
- Line 22: The TypeScript signature for clickNavLink is wrong: either make the
declaration return void or change the implementation to return the Cypress
Chainable so types match; recommended: update the declaration of
clickNavLink(path: [string, string?]) to return void (since the implementation
currently does not return the chain), or if chaining is required, modify the
implementation in clickNavLink to return the final Cypress command chain (ensure
the final expression inside .within(...) is returned) so the declared Chainable
type is satisfied.
- Line 21: The declaration for clickBtn in selectors.ts currently requires
btnTxt: string but the implementation accepts an optional parameter; update the
type signature for clickBtn to make btnTxt optional (btnTxt?: string) so it
matches the implementation (clickBtn) and any other declarations/overloads in
the same file that reference clickBtn to avoid the type mismatch.
In `@ui-tests-cy/tests/nmstate/physical.cy.ts`:
- Around line 21-25: The current selector uses cy.contains('td',
PHYSICAL_NETWORK_TEST_NAME).closest('tbody') which can select the wrong row's
expand button; instead scope to the specific row by using .closest('tr') from
the cell that contains PHYSICAL_NETWORK_TEST_NAME and then locate the expand
button within that row (e.g., the first cell's button) before clicking; update
the selector chain that references PHYSICAL_NETWORK_TEST_NAME to use
.closest('tr') and then .find(...) to ensure you target the row-scoped expand
button.
---
Duplicate comments:
In `@cleanup.sh`:
- Around line 7-12: The cleanup function uses a different default TEST_NS and
omits namespace scope when deleting resources; change the TEST_NS default to
match setup.sh (use TEST_NS="${TEST_NS:-cy-test-ns}") and update the oc delete
invocations inside cleanup (the commands in function cleanup and the second oc
delete line) to include -n "${TEST_NS}" so deletes target the intended
namespace; keep --ignore-not-found and timeouts as-is and preserve the
2>/dev/null || true behavior if desired.
In `@setup.sh`:
- Around line 7-11: The TEST_NS default expansion is wrong and the variable is
unquoted; change the parameter expansion to use the fallback operator and quote
usages: replace occurrences of TEST_NS definition `${TEST_NS:cy-test-ns}` with
`${TEST_NS:-cy-test-ns}` and wrap variable references in the setup function (the
oc get namespace and oc create namespace calls inside the setup() function) with
double quotes around `${TEST_NS}` to prevent globbing/word-splitting.
In `@test-cypress.sh`:
- Around line 3-40: The script currently uses set +e so failures from setup() or
the Cypress run can be ignored; ensure failures are propagated by enabling
strict exit handling and exiting with the Cypress status: enable errexit
(replace set +e with set -e or add set -euo pipefail) and after running Cypress
use the captured test_exit_code to exit the script (add a final exit
"${test_exit_code}" so CI sees failures); also check the return value of setup
and cleanup calls (call setup and immediately exit on non-zero) to ensure setup
failures don't get swallowed.
In `@ui-tests-cy/package.json`:
- Around line 13-23: The devDependencies in package.json currently use caret
ranges (e.g., "`@cypress/webpack-preprocessor`", "cypress",
"cypress-multi-reporters", "dotenv", "esbuild-loader", "mocha-junit-reporter",
"mochawesome", "mochawesome-merge", "mochawesome-report-generator",
"typescript", "webpack"); change each version string to an exact pinned version
(remove the leading '^' so entries become e.g. "cypress": "15.3.0") and then
regenerate the lockfile by running your package manager install command to
persist the pinned versions.
In `@ui-tests-cy/support/commands.ts`:
- Around line 12-24: The deleteResource command is vulnerable because it
interpolates user-controlled strings into a shell command; fix it by validating
and sanitizing the three parameters in the deleteResource function (kind, name,
namespace) before calling cy.exec: enforce strict allowlists/regexes (e.g., only
letters, numbers, '-', '.', and length limits) or map known resource kinds to an
enum, reject anything that doesn't match, and only then build the command;
update the cy.exec calls in deleteResource to use those validated values (or
fail fast) so no unvalidated input is ever interpolated into the shell string.
In `@ui-tests-cy/support/selectors.ts`:
- Around line 29-34: The custom Cypress command 'byTestID' currently calls
cy.get(`[data-test="${selector}"]`, options) but does not return it, which
breaks Cypress chaining; update the command implementation for 'byTestID' (the
Cypress.Commands.add call) to return the result of cy.get(...) so callers can
chain .click(), .should(), etc. normally.
---
Nitpick comments:
In `@ui-tests-cy/tests/nmstate/physical.cy.ts`:
- Around line 4-7: Add an after hook to ensure created PhysicalNetwork resources
are removed after the suite: in the adminOnlyDescribe block that contains the
before hook, add an after(() => { ... }) that calls
cy.deleteResource(K8S_KIND.NNCP, PHYSICAL_NETWORK_TEST_NAME) (matching the
before) so the resource is cleaned up even if tests fail or are interrupted;
place this after hook alongside the existing before to guarantee teardown of
PHYSICAL_NETWORK_TEST_NAME.
- Around line 14-17: The test 'verify Physical network in list' depends on a
prior test creating the network; make it independent by adding setup that
ensures the resource exists before running: add a beforeEach (or move into a
nested describe with setup) that calls the creation helper used by the 'create
Physical network' flow (or an API utility) to create PHYSICAL_NETWORK_TEST_NAME,
then call cy.visitPhysicalNetworks() and assert presence; reference the test
name 'verify Physical network in list', the helper cy.visitPhysicalNetworks(),
and the constant PHYSICAL_NETWORK_TEST_NAME when locating where to add the
setup/creation logic.
- Around line 21-26: Replace the hard-coded cy.wait(2 * SECOND) after clicking
the row action so the test waits for a real DOM change: after clicking the
button found via cy.contains('td',
PHYSICAL_NETWORK_TEST_NAME).closest('tbody').find('td:first-child
button').first().click(), assert the expected expanded state (for example, that
the clicked button or its container has aria-expanded="true" or that a known
expanded-content element/class appears) with an explicit timeout so Cypress will
retry until the row is expanded instead of sleeping.
In `@ui-tests-cy/tests/nmstate/visit-pages.cy.ts`:
- Around line 14-17: The test "should navigate to Node network configuration"
uses cy.clickNavLink(['Networking', 'Node network configuration']) and then
asserts via cy.url(), which is inconsistent with other navigation tests that
assert the presence of an h1; update the test to assert the page heading instead
(e.g., check that the page contains an h1 with the expected text) by replacing
or augmenting the cy.url().should(...) assertion with an h1 existence/text
assertion targeting the Node network configuration page; if the page truly lacks
an h1, add a brief inline comment in this test explaining why URL assertion is
kept and consider adding a more specific DOM selector to assert page load
consistency.
- Around line 19-28: The conditional DOM check in the test "should navigate to
Virtual machine networks (if Virtualization present)" using
cy.get('`#page-sidebar`').then(...) is flaky; either read a Cypress env var like
HAS_VIRTUALIZATION (Cypress.env('HAS_VIRTUALIZATION')) at the top of the test
and branch to call cy.clickNavLink(['Virtualization', 'Virtual machine
networks']) and the subsequent cy.contains('h1', 'Virtual machine networks', {
timeout: MINUTE }) when true, or if you must detect via DOM, replace the
immediate .then() check with a robust wait/assertion (e.g.,
cy.get('`#page-sidebar`').should('exist').and('not.be.empty') before checking
.invoke('text') or use cy.contains('Virtualization', { timeout: MINUTE
}).then(...) ) so the sidebar is fully loaded before deciding to navigate or
skip.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 417d9e56-2535-499c-b6e0-83c0f66d2113
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonui-tests-cy/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (29)
.env.example.gitignorecleanup.shresearch-flakiness.shsetup.shtest-cypress.shui-tests-cy/.eslintrcui-tests-cy/cypress.config.jsui-tests-cy/package.jsonui-tests-cy/plugins/index.tsui-tests-cy/reporter-config.jsonui-tests-cy/support/commands.tsui-tests-cy/support/index.tsui-tests-cy/support/login.tsui-tests-cy/support/nav.tsui-tests-cy/support/selectors.tsui-tests-cy/tests/all.cy.tsui-tests-cy/tests/nmstate/nncp.cy.tsui-tests-cy/tests/nmstate/nns.cy.tsui-tests-cy/tests/nmstate/physical.cy.tsui-tests-cy/tests/nmstate/visit-pages.cy.tsui-tests-cy/tests/nmstate/vmn.cy.tsui-tests-cy/tests/setup/login.cy.tsui-tests-cy/tsconfig.jsonui-tests-cy/utils/const/base.tsui-tests-cy/views/nncp.tsui-tests-cy/views/physicalNetwork.tsui-tests-cy/views/selector-common.tsui-tests-cy/views/vmNetwork.ts
✅ Files skipped from review due to trivial changes (2)
- ui-tests-cy/views/selector-common.ts
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (17)
- ui-tests-cy/reporter-config.json
- ui-tests-cy/.eslintrc
- ui-tests-cy/plugins/index.ts
- ui-tests-cy/tsconfig.json
- ui-tests-cy/support/index.ts
- ui-tests-cy/views/nncp.ts
- ui-tests-cy/views/vmNetwork.ts
- ui-tests-cy/tests/all.cy.ts
- research-flakiness.sh
- ui-tests-cy/utils/const/base.ts
- ui-tests-cy/tests/nmstate/vmn.cy.ts
- ui-tests-cy/support/nav.ts
- ui-tests-cy/tests/nmstate/nncp.cy.ts
- ui-tests-cy/tests/nmstate/nns.cy.ts
- ui-tests-cy/views/physicalNetwork.ts
- ui-tests-cy/tests/setup/login.cy.ts
- ui-tests-cy/support/login.ts
lkladnit
force-pushed
the
qe-ui
branch
3 times, most recently
from
b602f4f to
cca7fc5
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
setup.sh (1)
11-11:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winQuote the second
${TEST_NS}reference for consistency.The first reference is quoted (
"${TEST_NS}"), but the second inoc create namespace ${TEST_NS}is not. While Kubernetes namespace names are restricted and unlikely to contain special characters, quoting ensures consistent defensive behavior and satisfies shellcheck.🔧 Proposed fix
- oc get namespace "${TEST_NS}" 2>/dev/null || oc create namespace ${TEST_NS} + oc get namespace "${TEST_NS}" 2>/dev/null || oc create namespace "${TEST_NS}"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@setup.sh` at line 11, The second reference to the TEST_NS variable in the oc create command is unquoted; update the oc create namespace ${TEST_NS} invocation to use the same quoted form as the oc get namespace "${TEST_NS}" (i.e., wrap the second ${TEST_NS} in double quotes) so both uses are consistent and safe against special characters.Source: Linters/SAST tools
cleanup.sh (1)
11-12:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winAdd explicit namespace targeting to all
oc deletecommands.Neither delete command specifies a namespace with
-n "${TEST_NS}", so they will target the current kubectl context's default namespace rather than the test namespace. Resources created inTEST_NSby the test suite will not be cleaned up.🔧 Proposed fix
- oc delete nncp test-form-nncp test-vm-nncp example test-nncp-physical-network test-vm-physical-network --ignore-not-found --wait=true --timeout=180s + oc delete nncp test-form-nncp test-vm-nncp example test-nncp-physical-network test-vm-physical-network -n "${TEST_NS}" --ignore-not-found --wait=true --timeout=180s - oc delete --ignore-not-found=true virtualmachinenetwork test-vmn-project test-vmn-labeled --wait=true --timeout=60s 2>/dev/null || true + oc delete --ignore-not-found=true virtualmachinenetwork test-vmn-project test-vmn-labeled -n "${TEST_NS}" --wait=true --timeout=60s 2>/dev/null || true🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cleanup.sh` around lines 11 - 12, The oc delete commands in cleanup.sh (the lines deleting nncp resources and virtualmachinenetworks) lack explicit namespace targeting so they operate in the current kubectl context; update both commands to include -n "${TEST_NS}" (e.g., oc delete nncp ... -n "${TEST_NS}" --ignore-not-found --wait=... and oc delete --ignore-not-found=true virtualmachinenetwork ... -n "${TEST_NS}" --wait=...) so the test namespace is explicitly used when deleting test-form-nncp, test-vm-nncp, example, test-nncp-physical-network, test-vm-physical-network, test-vmn-project and test-vmn-labeled.ui-tests-cy/support/commands.ts (1)
12-23:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUnvalidated shell interpolation in
deleteResourceis still command-injection prone.Line 14 and Line 21 interpolate
kind,name, andnamespacedirectly intocy.exec(...). This remains exploitable if any caller passes unexpected shell metacharacters.🛡️ Minimal hardening diff (allow-list validation before exec)
Cypress.Commands.add('deleteResource', (kind: string, name: string, namespace?: string) => { + const safeKind = /^[a-z0-9]([a-z0-9._-]*[a-z0-9])?$/i; + const safeNameOrNamespace = /^[a-z0-9]([a-z0-9.-]*[a-z0-9])?$/i; + + if (!safeKind.test(kind) || !safeNameOrNamespace.test(name)) { + throw new Error('Invalid kind or name passed to deleteResource'); + } + if (namespace && !safeNameOrNamespace.test(namespace)) { + throw new Error('Invalid namespace passed to deleteResource'); + } + if (!namespace) { cy.exec(`oc delete --ignore-not-found=true ${kind} ${name} --wait=true --timeout=300s`, { failOnNonZeroExit: false, timeout: 5 * MINUTE, }); return; }As per coding guidelines, "Command: no shell=True, os.system, or backtick exec with user input" and "Validate at trust boundaries with allow-lists, not deny-lists."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui-tests-cy/support/commands.ts` around lines 12 - 23, The deleteResource command is vulnerable to shell injection by interpolating unvalidated kind, name, and namespace into cy.exec; update the deleteResource function to validate inputs before building the command: enforce kind against an allow-list (e.g., allowedKinds array containing known resource types) and validate name and namespace with a strict regex matching Kubernetes DNS-1123 label rules (only lower-case alphanumerics and hyphens, no leading/trailing hyphen), fail the test or throw if validation fails, then construct the oc delete command using the validated/sanitized strings (kind, name, namespace) for the cy.exec calls; reference deleteResource and the variables kind, name, namespace when making these changes.Sources: Coding guidelines, Linters/SAST tools
🧹 Nitpick comments (2)
test-cypress.sh (1)
8-14: ⚡ Quick winAdd a default case to handle invalid flags.
The
getoptscase statement lacks a*)default branch, so unrecognized flags are silently ignored. Adding a default case improves user experience by flagging typos or incorrect usage.♻️ Proposed improvement
while getopts g:s: flag do case "${flag}" in g) gui=${OPTARG};; s) spec=${OPTARG};; + *) echo "Invalid flag: -${OPTARG}" >&2; exit 1;; esac done🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test-cypress.sh` around lines 8 - 14, The case handling for getopts currently only matches g and s and silently ignores unknown flags; update the case statement that processes the getopts "flag" to add a default branch (*) that prints a short usage/error message (mentioning expected flags like -g and -s), optionally referencing OPTARG or "$flag" to show the invalid option, and exit with a non-zero status so invalid flags (when parsing gui and spec) are surfaced to the user.Source: Linters/SAST tools
ui-tests-cy/tests/nmstate/nncp.cy.ts (1)
22-42: ⚡ Quick winReduce cross-test state coupling in this suite.
nodes summary,page controls,edit, anddeletedepend on artifacts created by earlieritcases. This makes isolated reruns brittle and increases flake risk; prefer per-test setup (beforeEach) or a single end-to-end scenario for dependent steps.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui-tests-cy/tests/nmstate/nncp.cy.ts` around lines 22 - 42, Tests in this suite are coupling state across it blocks causing brittle runs; either create the NNCP fixture in a beforeEach or collapse dependent steps into one end-to-end it. Add a beforeEach that ensures NNCP_TEST_NAME exists (call whatever helper creates the NNCP) and cleans up or resets filters before each test, then keep the existing assertions that reference NNCP_TEST_NAME, itemFilter, editNNCP, deleteNNCP and cy.visitNNCP; alternatively wrap the sequence of assertions and actions (nodes summary, page controls, editNNCP + cy.visitNNCP, deleteNNCP) into a single it so the setup/teardown are local to that scenario. Ensure the helpers used for creation/cleanup are idempotent so individual tests can run in isolation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cleanup.sh`:
- Line 7: Uncomment and restore the default TEST_NS assignment so the cleanup
function has a fallback namespace: remove the leading comment from the line that
sets TEST_NS using parameter expansion (TEST_NS="${TEST_NS:-cy-test-ns}") in
cleanup.sh, ensuring any references to TEST_NS inside the cleanup function (and
related logic) will use the provided env value or fall back to "cy-test-ns" when
.env does not supply it.
In `@test-cypress.sh`:
- Line 36: The conditional uses an unquoted variable `${test_exit_code}` which
can break under certain shell settings and triggers shellcheck; update the if
test that reads `if [ ${test_exit_code} -eq 0 ]; then` to quote the variable
reference (e.g., `if [ "${test_exit_code}" -eq 0 ]; then`) so `test_exit_code`
is safely handled; look for the `if` statement controlling the test exit path
and the `test_exit_code` variable in the script to make this change.
In `@ui-tests-cy/tests/nmstate/nncp.cy.ts`:
- Line 14: The test currently uses a global selector
cy.contains(NNCP_TEST_NAME).should('exist') which can match unrelated text;
change it to scope the lookup to the NNCP table row by using a row-scoped
contains call (e.g., cy.contains('tr', NNCP_TEST_NAME).should('exist')) so it
matches the NNCP table row consistently with the other row-scoped checks that
reference NNCP_TEST_NAME at lines below; update the assertion where
NNCP_TEST_NAME is used to use the 'tr' scoped contains call.
---
Duplicate comments:
In `@cleanup.sh`:
- Around line 11-12: The oc delete commands in cleanup.sh (the lines deleting
nncp resources and virtualmachinenetworks) lack explicit namespace targeting so
they operate in the current kubectl context; update both commands to include -n
"${TEST_NS}" (e.g., oc delete nncp ... -n "${TEST_NS}" --ignore-not-found
--wait=... and oc delete --ignore-not-found=true virtualmachinenetwork ... -n
"${TEST_NS}" --wait=...) so the test namespace is explicitly used when deleting
test-form-nncp, test-vm-nncp, example, test-nncp-physical-network,
test-vm-physical-network, test-vmn-project and test-vmn-labeled.
In `@setup.sh`:
- Line 11: The second reference to the TEST_NS variable in the oc create command
is unquoted; update the oc create namespace ${TEST_NS} invocation to use the
same quoted form as the oc get namespace "${TEST_NS}" (i.e., wrap the second
${TEST_NS} in double quotes) so both uses are consistent and safe against
special characters.
In `@ui-tests-cy/support/commands.ts`:
- Around line 12-23: The deleteResource command is vulnerable to shell injection
by interpolating unvalidated kind, name, and namespace into cy.exec; update the
deleteResource function to validate inputs before building the command: enforce
kind against an allow-list (e.g., allowedKinds array containing known resource
types) and validate name and namespace with a strict regex matching Kubernetes
DNS-1123 label rules (only lower-case alphanumerics and hyphens, no
leading/trailing hyphen), fail the test or throw if validation fails, then
construct the oc delete command using the validated/sanitized strings (kind,
name, namespace) for the cy.exec calls; reference deleteResource and the
variables kind, name, namespace when making these changes.
---
Nitpick comments:
In `@test-cypress.sh`:
- Around line 8-14: The case handling for getopts currently only matches g and s
and silently ignores unknown flags; update the case statement that processes the
getopts "flag" to add a default branch (*) that prints a short usage/error
message (mentioning expected flags like -g and -s), optionally referencing
OPTARG or "$flag" to show the invalid option, and exit with a non-zero status so
invalid flags (when parsing gui and spec) are surfaced to the user.
In `@ui-tests-cy/tests/nmstate/nncp.cy.ts`:
- Around line 22-42: Tests in this suite are coupling state across it blocks
causing brittle runs; either create the NNCP fixture in a beforeEach or collapse
dependent steps into one end-to-end it. Add a beforeEach that ensures
NNCP_TEST_NAME exists (call whatever helper creates the NNCP) and cleans up or
resets filters before each test, then keep the existing assertions that
reference NNCP_TEST_NAME, itemFilter, editNNCP, deleteNNCP and cy.visitNNCP;
alternatively wrap the sequence of assertions and actions (nodes summary, page
controls, editNNCP + cy.visitNNCP, deleteNNCP) into a single it so the
setup/teardown are local to that scenario. Ensure the helpers used for
creation/cleanup are idempotent so individual tests can run in isolation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 5d910e36-26a9-4beb-af1a-553db738f1c1
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonui-tests-cy/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (29)
.env.example.gitignorecleanup.shresearch-flakiness.shsetup.shtest-cypress.shui-tests-cy/.eslintrcui-tests-cy/cypress.config.jsui-tests-cy/package.jsonui-tests-cy/plugins/index.tsui-tests-cy/reporter-config.jsonui-tests-cy/support/commands.tsui-tests-cy/support/index.tsui-tests-cy/support/login.tsui-tests-cy/support/nav.tsui-tests-cy/support/selectors.tsui-tests-cy/tests/all.cy.tsui-tests-cy/tests/nmstate/nncp.cy.tsui-tests-cy/tests/nmstate/nns.cy.tsui-tests-cy/tests/nmstate/physical.cy.tsui-tests-cy/tests/nmstate/visit-pages.cy.tsui-tests-cy/tests/nmstate/vmn.cy.tsui-tests-cy/tests/setup/login.cy.tsui-tests-cy/tsconfig.jsonui-tests-cy/utils/const/base.tsui-tests-cy/views/nncp.tsui-tests-cy/views/physicalNetwork.tsui-tests-cy/views/selector-common.tsui-tests-cy/views/vmNetwork.ts
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (21)
- ui-tests-cy/tsconfig.json
- ui-tests-cy/reporter-config.json
- ui-tests-cy/tests/nmstate/vmn.cy.ts
- ui-tests-cy/cypress.config.js
- ui-tests-cy/utils/const/base.ts
- ui-tests-cy/support/index.ts
- ui-tests-cy/tests/nmstate/visit-pages.cy.ts
- ui-tests-cy/views/selector-common.ts
- ui-tests-cy/tests/all.cy.ts
- ui-tests-cy/support/login.ts
- ui-tests-cy/package.json
- ui-tests-cy/support/nav.ts
- ui-tests-cy/views/nncp.ts
- ui-tests-cy/.eslintrc
- ui-tests-cy/tests/nmstate/nns.cy.ts
- research-flakiness.sh
- ui-tests-cy/tests/nmstate/physical.cy.ts
- ui-tests-cy/views/vmNetwork.ts
- ui-tests-cy/support/selectors.ts
- ui-tests-cy/views/physicalNetwork.ts
- ui-tests-cy/plugins/index.ts
| # Sources .env for namespace names | ||
|
|
||
| source .env 2>/dev/null || true | ||
| #TEST_NS="${TEST_NS:-cy-test-ns}" |
There was a problem hiding this comment.
Uncomment and use the TEST_NS default.
Line 7 is commented out, so TEST_NS has no fallback when .env doesn't provide it. Without this line, the cleanup function cannot reliably determine which namespace to target.
🔧 Proposed fix
-#TEST_NS="${TEST_NS:-cy-test-ns}"
+TEST_NS="${TEST_NS:-cy-test-ns}"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| #TEST_NS="${TEST_NS:-cy-test-ns}" | |
| TEST_NS="${TEST_NS:-cy-test-ns}" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cleanup.sh` at line 7, Uncomment and restore the default TEST_NS assignment
so the cleanup function has a fallback namespace: remove the leading comment
from the line that sets TEST_NS using parameter expansion
(TEST_NS="${TEST_NS:-cy-test-ns}") in cleanup.sh, ensuring any references to
TEST_NS inside the cleanup function (and related logic) will use the provided
env value or fall back to "cy-test-ns" when .env does not supply it.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
cleanup.sh (2)
7-7:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUncomment the TEST_NS default assignment.
Without this line,
TEST_NShas no fallback when.envis missing or doesn't define it, causing thecleanup()function to operate in an undefined or incorrect namespace. This matches a previous review comment that remains unaddressed.🔧 Proposed fix
-#TEST_NS="${TEST_NS:-cy-test-ns}" +TEST_NS="${TEST_NS:-cy-test-ns}"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cleanup.sh` at line 7, Uncomment the default TEST_NS assignment so the TEST_NS environment variable has a fallback; restore the line setting TEST_NS="${TEST_NS:-cy-test-ns}" (or equivalent) so cleanup() uses a defined namespace when .env is missing or doesn't define TEST_NS; ensure no other logic overrides this default and that cleanup() references TEST_NS consistently.
11-12:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd explicit namespace scoping to all
oc deletecommands.Both delete commands lack
-n "${TEST_NS}"flags, so they target the current kubectl context namespace instead of the intended test namespace. This can leave test resources in${TEST_NS}or accidentally delete resources in production namespaces.🔧 Proposed fix
- oc delete nncp test-form-nncp test-vm-nncp example test-nncp-physical-network test-vm-physical-network --ignore-not-found --wait=true --timeout=180s - oc delete --ignore-not-found=true virtualmachinenetwork test-vmn-project test-vmn-labeled --wait=true --timeout=60s 2>/dev/null || true + oc delete nncp test-form-nncp test-vm-nncp example test-nncp-physical-network test-vm-physical-network -n "${TEST_NS}" --ignore-not-found --wait=true --timeout=180s + oc delete --ignore-not-found=true virtualmachinenetwork test-vmn-project test-vmn-labeled -n "${TEST_NS}" --wait=true --timeout=60s 2>/dev/null || true🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cleanup.sh` around lines 11 - 12, The oc delete commands (the lines invoking oc delete nncp ... and oc delete --ignore-not-found=true virtualmachinenetwork ...) are missing explicit namespace scoping; update both commands to include -n "${TEST_NS}" so they operate against the intended TEST_NS instead of the current kubectl context namespace, ensuring each oc delete invocation (for resources like nncp, virtualmachinenetwork, test-form-nncp, test-vm-nncp, etc.) is run with -n "${TEST_NS}" and preserving the existing flags (--ignore-not-found, --wait, --timeout, and redirection/|| true).
🧹 Nitpick comments (4)
setup.sh (1)
11-11: ⚡ Quick winQuote the variable in
oc create namespace.The second
${TEST_NS}reference should be quoted to prevent globbing and word splitting, satisfying shellcheck SC2086 and matching the quoting used in theoc getcommand.🔧 Proposed fix
- oc get namespace "${TEST_NS}" 2>/dev/null || oc create namespace ${TEST_NS} + oc get namespace "${TEST_NS}" 2>/dev/null || oc create namespace "${TEST_NS}"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@setup.sh` at line 11, The oc create command uses an unquoted variable; update the second reference so both commands use the same quoted variable form by changing oc create namespace ${TEST_NS} to use the quoted parameter for TEST_NS (i.e., quote the ${TEST_NS} in the oc create namespace invocation) so the oc get and oc create lines consistently use "${TEST_NS}".Source: Linters/SAST tools
.env.example (1)
3-4: 💤 Low valueConsider reordering keys alphabetically.
The static analysis tool suggests placing
HIDE_XHRbeforeTEST_NSfor consistency. While this doesn't affect functionality, alphabetical ordering can improve maintainability.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.env.example around lines 3 - 4, Reorder the environment variable keys in .env.example so they are alphabetically sorted: place HIDE_XHR before TEST_NS; update the file to list HIDE_XHR then TEST_NS to satisfy the static analysis ordering preference.Source: Linters/SAST tools
test-cypress.sh (2)
10-13: 💤 Low valueAdd a default case to the
getoptsswitch.Shellcheck SC2220 suggests adding a
*)case to handle invalid flags. Whilegetoptsitself handles unknown flags, an explicit default case improves error messaging and defensive coding.🔧 Proposed fix
case "${flag}" in g) gui=${OPTARG};; s) spec=${OPTARG};; + *) echo "Invalid option: -${OPTARG}" >&2; exit 1;; esac🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test-cypress.sh` around lines 10 - 13, The getopts case handling for variable "flag" (case "${flag}" in with branches g) gui and s) spec) lacks a default branch; add a *) default case to the case statement (alongside g) and s)) to catch invalid flags, print a clear error/usage message (e.g., "Invalid option: -${flag}") to stderr, and exit with a non-zero status so the script fails fast when unknown options are provided; ensure this integrates with any existing usage/help output if present.Source: Linters/SAST tools
36-36: ⚡ Quick winQuote
test_exit_codein the conditional.The variable should be quoted to satisfy shellcheck SC2086 and follow shell best practices. A previous review comment flagged this, but the code remains unquoted.
🔧 Proposed fix
- if [ ${test_exit_code} -eq 0 ]; then + if [ "${test_exit_code}" -eq 0 ]; then cleanup fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test-cypress.sh` at line 36, The conditional uses an unquoted variable expansion and should quote the test_exit_code variable to avoid word-splitting and satisfy shellcheck SC2086; update the if test in test-cypress.sh to reference test_exit_code as a quoted expansion (replace the unquoted ${test_exit_code} usage in the if [ ... -eq 0 ]; then conditional with a quoted expansion) so the comparison is safe when the variable is empty or contains spaces.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.env.example:
- Around line 1-4: Remove surrounding quotes from environment variable values in
.env.example so dotenv parsers don't include literal quotes; specifically change
BRIDGE_BASE_ADDRESS to an unquoted URL and BRIDGE_KUBEADMIN_PASSWORD to an empty
value (no quotes), and likewise remove quotes from TEST_NS and HIDE_XHR. This
will ensure config.baseUrl (used in ui-tests-cy/plugins/index.ts via
config.baseUrl) receives a proper URL string and empty-password checks work as
intended.
In `@setup.sh`:
- Line 7: The TEST_NS default in setup.sh ("TEST_NS") is mismatched with the
test default in ui-tests-cy/utils/const/base.ts (the default namespace constant
set to "nmstate-test-ns"); pick one consistent default and update the other so
both match—either change TEST_NS in setup.sh to "nmstate-test-ns" or change the
namespace default in base.ts to "cy-test-ns" so tests and setup use the same
namespace value.
In `@ui-tests-cy/support/nav.ts`:
- Line 17: The test contains a typo in the assertion: update the cy.contains
call in ui-tests-cy/support/nav.ts that currently looks for
'NodeNetworkConfigurationPolic' to the correct resource name
'NodeNetworkConfigurationPolicy' so the h1 lookup matches the expected title
(the statement to change is the cy.contains('h1',
'NodeNetworkConfigurationPolic', { timeout: MINUTE }).should('exist')
invocation).
In `@ui-tests-cy/support/selectors.ts`:
- Line 21: The TypeScript declaration for clickBtn is wrong: change its return
type from void to the Cypress Chainable type to match the implementation that
returns cy.contains(...).click(); update the declaration signature
clickBtn(btnTxt?: string): void to return Cypress.Chainable or
Chainable<JQuery<HTMLElement>> (consistent with other selector methods) so
callers can continue chaining; ensure the exported interface/type that contains
clickBtn reflects the new return type as well.
- Line 23: The TypeScript declaration for clickNextBtn is incorrect: it declares
clickNextBtn(): void but the implementation returns a Cypress Chainable; update
the function signature for clickNextBtn to return the appropriate Cypress type
(e.g., Cypress.Chainable<JQuery<HTMLElement>> or Cypress.Chainable<any>) to
match the implementation (or alternatively change the implementation to not
return the cy call), ensuring the declaration and the implementation for
clickNextBtn are consistent.
---
Duplicate comments:
In `@cleanup.sh`:
- Line 7: Uncomment the default TEST_NS assignment so the TEST_NS environment
variable has a fallback; restore the line setting
TEST_NS="${TEST_NS:-cy-test-ns}" (or equivalent) so cleanup() uses a defined
namespace when .env is missing or doesn't define TEST_NS; ensure no other logic
overrides this default and that cleanup() references TEST_NS consistently.
- Around line 11-12: The oc delete commands (the lines invoking oc delete nncp
... and oc delete --ignore-not-found=true virtualmachinenetwork ...) are missing
explicit namespace scoping; update both commands to include -n "${TEST_NS}" so
they operate against the intended TEST_NS instead of the current kubectl context
namespace, ensuring each oc delete invocation (for resources like nncp,
virtualmachinenetwork, test-form-nncp, test-vm-nncp, etc.) is run with -n
"${TEST_NS}" and preserving the existing flags (--ignore-not-found, --wait,
--timeout, and redirection/|| true).
---
Nitpick comments:
In @.env.example:
- Around line 3-4: Reorder the environment variable keys in .env.example so they
are alphabetically sorted: place HIDE_XHR before TEST_NS; update the file to
list HIDE_XHR then TEST_NS to satisfy the static analysis ordering preference.
In `@setup.sh`:
- Line 11: The oc create command uses an unquoted variable; update the second
reference so both commands use the same quoted variable form by changing oc
create namespace ${TEST_NS} to use the quoted parameter for TEST_NS (i.e., quote
the ${TEST_NS} in the oc create namespace invocation) so the oc get and oc
create lines consistently use "${TEST_NS}".
In `@test-cypress.sh`:
- Around line 10-13: The getopts case handling for variable "flag" (case
"${flag}" in with branches g) gui and s) spec) lacks a default branch; add a *)
default case to the case statement (alongside g) and s)) to catch invalid flags,
print a clear error/usage message (e.g., "Invalid option: -${flag}") to stderr,
and exit with a non-zero status so the script fails fast when unknown options
are provided; ensure this integrates with any existing usage/help output if
present.
- Line 36: The conditional uses an unquoted variable expansion and should quote
the test_exit_code variable to avoid word-splitting and satisfy shellcheck
SC2086; update the if test in test-cypress.sh to reference test_exit_code as a
quoted expansion (replace the unquoted ${test_exit_code} usage in the if [ ...
-eq 0 ]; then conditional with a quoted expansion) so the comparison is safe
when the variable is empty or contains spaces.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 7bc3ecff-8478-4a24-847f-51ed10b901a2
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonui-tests-cy/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (29)
.env.example.gitignorecleanup.shresearch-flakiness.shsetup.shtest-cypress.shui-tests-cy/.eslintrcui-tests-cy/cypress.config.jsui-tests-cy/package.jsonui-tests-cy/plugins/index.tsui-tests-cy/reporter-config.jsonui-tests-cy/support/commands.tsui-tests-cy/support/index.tsui-tests-cy/support/login.tsui-tests-cy/support/nav.tsui-tests-cy/support/selectors.tsui-tests-cy/tests/all.cy.tsui-tests-cy/tests/nmstate/nncp.cy.tsui-tests-cy/tests/nmstate/nns.cy.tsui-tests-cy/tests/nmstate/physical.cy.tsui-tests-cy/tests/nmstate/visit-pages.cy.tsui-tests-cy/tests/nmstate/vmn.cy.tsui-tests-cy/tests/setup/login.cy.tsui-tests-cy/tsconfig.jsonui-tests-cy/utils/const/base.tsui-tests-cy/views/nncp.tsui-tests-cy/views/physicalNetwork.tsui-tests-cy/views/selector-common.tsui-tests-cy/views/vmNetwork.ts
✅ Files skipped from review due to trivial changes (4)
- ui-tests-cy/tsconfig.json
- .gitignore
- ui-tests-cy/.eslintrc
- ui-tests-cy/tests/all.cy.ts
🚧 Files skipped from review as they are similar to previous changes (14)
- ui-tests-cy/tests/nmstate/physical.cy.ts
- ui-tests-cy/reporter-config.json
- ui-tests-cy/tests/setup/login.cy.ts
- ui-tests-cy/cypress.config.js
- ui-tests-cy/views/selector-common.ts
- ui-tests-cy/tests/nmstate/vmn.cy.ts
- ui-tests-cy/support/index.ts
- ui-tests-cy/support/login.ts
- ui-tests-cy/package.json
- ui-tests-cy/tests/nmstate/nncp.cy.ts
- ui-tests-cy/plugins/index.ts
- research-flakiness.sh
- ui-tests-cy/views/nncp.ts
- ui-tests-cy/views/vmNetwork.ts
lkladnit
force-pushed
the
qe-ui
branch
3 times, most recently
from
02f0305 to
6d5ac38
Compare
Author
|
All 22 tests pass on Jenkins CI: https://jenkins-csb-cnvqe-main.dno.corp.redhat.com/job/dev-test-kubevirt-console/241/ |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
ui-tests-cy/tests/nmstate/nns.cy.ts (2)
4-16: 💤 Low valueConsider more idiomatic Cypress selectors for button text matching.
Lines 12-14 use
.filter(':contains("...")), which is jQuery syntax. Cypress provides more idiomatic alternatives:
- Use
cy.contains('td button', 'ethernet')to find buttons with specific text.- If available, use
data-testattributes for more stable selectors.♻️ Alternative approach
cy.contains('table tbody tr', 'worker').first().within(() => { cy.get('td button').should('have.length.at.least', 3); - cy.get('td button').filter(':contains("ethernet")').should('exist'); - cy.get('td button').filter(':contains("linux-bridge")').should('exist'); - cy.get('td button').filter(':contains("ovs-bridge")').should('exist'); + cy.contains('td button', 'ethernet').should('exist'); + cy.contains('td button', 'linux-bridge').should('exist'); + cy.contains('td button', 'ovs-bridge').should('exist'); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui-tests-cy/tests/nmstate/nns.cy.ts` around lines 4 - 16, Replace the jQuery-style .filter(':contains("...")') selectors in the "verify bridge on NodeNetworkState" test with Cypress-native text selectors: use cy.contains('td button', 'ethernet'), cy.contains('td button', 'linux-bridge'), and cy.contains('td button', 'ovs-bridge') inside the same within() block (or, when available, switch to stable data-test attributes on the buttons and use cy.get('[data-test="..."]') for each button) so the test uses idiomatic Cypress selectors and avoids jQuery-specific filtering.
18-31: 💤 Low valueConsider more stable selectors for expanded state.
Lines 24-25 and 29-30 use
'[class*="expanded-true"], tbody.pf-m-expanded', which relies on PatternFly implementation details (class names and modifiers). These selectors are brittle and could break with PatternFly updates.If possible, use
data-testattributes or ARIA states ([aria-expanded="true"]) for more stable selectors.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui-tests-cy/tests/nmstate/nns.cy.ts` around lines 18 - 31, The test "verify expand all and collapse all" uses brittle selectors ('[class*="expanded-true"], tbody.pf-m-expanded') to detect expanded rows; update the checks in that it block to use stable attributes instead — prefer an explicit data-test attribute like data-test="row-expanded" on expandable rows or use ARIA state selectors such as [aria-expanded="true"] for the expand assertion and [aria-expanded="false"] or absence for the collapse assertion; change the two cy.get calls that currently reference the class-based selector to the chosen stable selector so the test queries the semantic/data-test/ARIA state rather than PatternFly class names.ui-tests-cy/views/vmNetwork.ts (4)
63-63: ⚡ Quick winRemove
{ force: true }from checkbox interaction.Line 63 uses
{ force: true }to click the project checkbox, bypassing Cypress's actionability checks. This can hide real UI issues such as the checkbox being covered by another element or not actually clickable.🔒 Recommended fix
- cy.contains('li', projectName).find('input[type="checkbox"]').click({ force: true }); + cy.contains('li', projectName).find('input[type="checkbox"]').should('be.visible').click();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui-tests-cy/views/vmNetwork.ts` at line 63, Remove the bypass of Cypress actionability checks on the project checkbox: replace the call to cy.contains('li', projectName).find('input[type="checkbox"]').click({ force: true }) with an actionable interaction such as cy.contains('li', projectName).find('input[type="checkbox"]').should('be.visible').check() (or .click() after the should('be.visible') assertion) so the test fails if the checkbox is not interactable instead of forcing the click.
17-31: ⚡ Quick winRemove
{ force: true }to respect Cypress actionability checks.Line 26 uses
{ force: true }to click the "Create physical network" button, bypassing Cypress's built-in checks for visibility, enabled state, and coverage by other elements. This can hide real UI issues.🔒 Recommended fix
- cy.contains('button', 'Create physical network').click({ force: true }); + cy.contains('button', 'Create physical network').should('be.visible').and('be.enabled').click();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui-tests-cy/views/vmNetwork.ts` around lines 17 - 31, The click using { force: true } in attemptToCreateVMNetwork should be replaced with an actionable click that respects Cypress checks: locate the "Create physical network" button via cy.contains('button', 'Create physical network') and ensure it's visible and enabled (e.g., .should('be.visible').and('not.be.disabled')) and/or scroll into view before calling .click(), so the flow in navigateToVMN → cy.get(itemCreateBtn) → fillPhysicalNetworkWizard remains the same but without bypassing actionability; adjust waits/timeouts around fillPhysicalNetworkWizard and the subsequent navigateToVMN call if needed to make the button reliably actionable.
41-80: ⚡ Quick winReplace hardcoded waits with explicit assertions.
Lines 41, 51, 62, and 80 use
cy.wait()with fixed time delays. This is a Cypress anti-pattern that makes tests slower and unreliable.
- Line 41: Remove the 2-second wait; the input visibility check on line 43 is sufficient.
- Lines 51, 62: Instead of waiting 1 second after typing, wait for the dropdown/combobox options to be visible.
- Line 80: Instead of waiting 5 seconds after clicking "Create," wait for navigation or a success indicator.
⚡ Recommended fix
cy.get(itemCreateBtn).click(); cy.contains('h1', 'Create virtual machine network', { timeout: MINUTE }).should('exist'); - cy.wait(2 * SECOND); cy.get(vmnNameInput, { timeout: MINUTE })cy.get('input[role="combobox"]').first().click(); cy.get('input[role="combobox"]').first().clear().type(physicalNetworkName); - cy.wait(SECOND); + cy.get('button[role="option"]').should('be.visible'); cy.get('button[role="option"]').contains(physicalNetworkName).click();cy.get('input[role="combobox"][aria-label="Type to filter"]').first().click(); cy.get('input[role="combobox"][aria-label="Type to filter"]').first().clear().type(projectName); - cy.wait(SECOND); + cy.contains('li', projectName).should('be.visible'); cy.contains('li', projectName).find('input[type="checkbox"]').click({ force: true });cy.contains(submitBtn, 'Create').click(); - cy.wait(5 * SECOND); + // Wait for success/error state or navigation cy.get('body').then(($body) => {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui-tests-cy/views/vmNetwork.ts` around lines 41 - 80, Remove hardcoded cy.waits and replace them with explicit assertions: drop the initial cy.wait(2 * SECOND) before interacting with vmnNameInput because the existing cy.get(vmnNameInput).should('be.visible') is sufficient; after typing into the physical network combobox (the first 'input[role="combobox"]') replace cy.wait(SECOND) with an assertion that the expected option/button[role="option"] containing physicalNetworkName is visible (e.g., cy.get('button[role="option"]').contains(physicalNetworkName).should('be.visible') before clicking); similarly after typing into the project combobox (input[role="combobox"][aria-label="Type to filter"]) assert that the matching li or checkbox option for projectName is visible before clicking it; and after clicking cy.contains(submitBtn, 'Create') remove cy.wait(5 * SECOND) and instead wait for the post-submit success indicator or navigation (e.g., assert visibility of a toast, a "1 project selected" update, or a unique page element) to confirm completion.
93-97: ⚡ Quick winReplace hardcoded waits in delete flow.
Lines 93, 95, and 97 use fixed time delays. Consider replacing these with explicit assertions:
- Line 93: Wait for the modal to be visible instead of a fixed 2-second delay.
- Line 95: Wait for the checkbox to be checked instead of a fixed 1-second delay.
- Line 97: Wait for the modal to close or the row to disappear instead of a fixed 3-second delay.
⚡ Recommended fix
cy.contains('button', 'Delete').click(); - cy.wait(2 * SECOND); + cy.get('`#delete-vm-network-modal-acknowledge-checkbox`').should('be.visible'); cy.get('`#delete-vm-network-modal-acknowledge-checkbox`').click({ force: true }); - cy.wait(SECOND); + cy.get('`#delete-vm-network-modal-acknowledge-checkbox`').should('be.checked'); cy.get('button').contains('Delete').last().click({ force: true }); - cy.wait(3 * SECOND); + cy.get('`#delete-vm-network-modal-acknowledge-checkbox`').should('not.exist');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui-tests-cy/views/vmNetwork.ts` around lines 93 - 97, The three hardcoded cy.waits should be replaced with explicit assertions: wait for the delete modal to appear (use an assertion that the modal is visible before interacting instead of cy.wait(2 * SECOND)), replace the post-click wait for '`#delete-vm-network-modal-acknowledge-checkbox`' with an assertion that the checkbox is checked (e.g., assert checked after .click({ force: true })), and replace the final cy.wait(3 * SECOND) with an assertion that the modal has closed or the deleted VM row is gone (e.g., assert the modal is not visible or the row representing the VM no longer exists after clicking the 'Delete' button). Use the existing selectors '`#delete-vm-network-modal-acknowledge-checkbox`' and the button containing 'Delete' to locate elements and rely on visibility/checked/existence assertions instead of fixed delays.ui-tests-cy/views/physicalNetwork.ts (1)
11-44: ⚡ Quick winReplace hardcoded waits with explicit assertions.
Lines 19 and 43 use
cy.wait()with fixed time delays. This is a Cypress anti-pattern that makes tests slower and still prone to timing issues.
- Line 19: Instead of waiting 2 seconds for the "auto-generated name to settle," assert that the input has a non-empty value before clearing:
cy.get(physicalNetworkNameInput).should('have.value').and('not.be.empty').- Line 43: Instead of waiting 3 seconds after clicking "Create network," wait for a success indicator such as navigation to the policy details page or a success alert.
⚡ Recommended fix
// Step 1: Network identity cy.get(physicalNetworkNameInput, { timeout: MINUTE }).should('be.visible'); - // Wait for form auto-generated name to settle before clearing - cy.wait(2 * SECOND); + cy.get(physicalNetworkNameInput).should('have.value').and('not.be.empty'); cy.get(physicalNetworkNameInput).clear();// Step 5: Review and Create cy.contains('button', 'Create network', { timeout: MINUTE }).should('be.visible'); cy.contains('button', 'Create network').click(); - cy.wait(3 * SECOND); + // Wait for navigation or success indicator instead of fixed delay🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui-tests-cy/views/physicalNetwork.ts` around lines 11 - 44, In fillPhysicalNetworkWizard replace the two hard waits with explicit assertions: for the initial wait, before clearing physicalNetworkNameInput assert the input has a non-empty value (e.g., cy.get(physicalNetworkNameInput).should('have.value').and('not.be.empty')) then clear/type as before; for the post-create wait, after clicking the Create network button remove cy.wait(3 * SECOND) and instead wait for a deterministic success indicator such as a success toast or navigation to the new network/policy details (e.g., assert a success alert is visible or the details header/route for the created network appears) so the flow uses physicalNetworkNameInput, submitBtn and the success selector instead of fixed time delays.ui-tests-cy/tests/nmstate/vmn.cy.ts (1)
28-32: 💤 Low valueConsider extracting hardcoded label to a constant.
Line 29 contains a hardcoded label string
'kubernetes.io/metadata.name=default'. Consider extracting this to a named constant (e.g.,VM_NETWORK_LABEL) at the top of the file or in the view helper for better maintainability.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui-tests-cy/tests/nmstate/vmn.cy.ts` around lines 28 - 32, Extract the hardcoded label string 'kubernetes.io/metadata.name=default' into a descriptive constant (e.g., VM_NETWORK_LABEL) and use that constant in the createVMNetwork call; update the top of the test file (or the shared view helper) to export the new VM_NETWORK_LABEL constant and replace occurrences in this file where createVMNetwork is invoked (reference symbols: createVMNetwork, VM_NETWORK_LABELED_NAME, REQ_PHYSICAL_NETWORK_NAME) so the label is maintained in one place for reuse and easier modification.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test-cypress.sh`:
- Around line 3-7: The script currently enables xtrace with "set -x" before
sourcing "./cleanup.sh" and "./setup.sh", which may echo secrets; change flow to
disable tracing around those sources: call "set +x" immediately before "source
./cleanup.sh" and "source ./setup.sh", then restore tracing with "set -x"
afterwards (or move the initial "set -x" to after the two source lines). Ensure
you still preserve "set +e" behavior and reference the existing tokens "set -x",
"set +e", "source ./cleanup.sh", and "source ./setup.sh" when making the change.
---
Nitpick comments:
In `@ui-tests-cy/tests/nmstate/nns.cy.ts`:
- Around line 4-16: Replace the jQuery-style .filter(':contains("...")')
selectors in the "verify bridge on NodeNetworkState" test with Cypress-native
text selectors: use cy.contains('td button', 'ethernet'), cy.contains('td
button', 'linux-bridge'), and cy.contains('td button', 'ovs-bridge') inside the
same within() block (or, when available, switch to stable data-test attributes
on the buttons and use cy.get('[data-test="..."]') for each button) so the test
uses idiomatic Cypress selectors and avoids jQuery-specific filtering.
- Around line 18-31: The test "verify expand all and collapse all" uses brittle
selectors ('[class*="expanded-true"], tbody.pf-m-expanded') to detect expanded
rows; update the checks in that it block to use stable attributes instead —
prefer an explicit data-test attribute like data-test="row-expanded" on
expandable rows or use ARIA state selectors such as [aria-expanded="true"] for
the expand assertion and [aria-expanded="false"] or absence for the collapse
assertion; change the two cy.get calls that currently reference the class-based
selector to the chosen stable selector so the test queries the
semantic/data-test/ARIA state rather than PatternFly class names.
In `@ui-tests-cy/tests/nmstate/vmn.cy.ts`:
- Around line 28-32: Extract the hardcoded label string
'kubernetes.io/metadata.name=default' into a descriptive constant (e.g.,
VM_NETWORK_LABEL) and use that constant in the createVMNetwork call; update the
top of the test file (or the shared view helper) to export the new
VM_NETWORK_LABEL constant and replace occurrences in this file where
createVMNetwork is invoked (reference symbols: createVMNetwork,
VM_NETWORK_LABELED_NAME, REQ_PHYSICAL_NETWORK_NAME) so the label is maintained
in one place for reuse and easier modification.
In `@ui-tests-cy/views/physicalNetwork.ts`:
- Around line 11-44: In fillPhysicalNetworkWizard replace the two hard waits
with explicit assertions: for the initial wait, before clearing
physicalNetworkNameInput assert the input has a non-empty value (e.g.,
cy.get(physicalNetworkNameInput).should('have.value').and('not.be.empty')) then
clear/type as before; for the post-create wait, after clicking the Create
network button remove cy.wait(3 * SECOND) and instead wait for a deterministic
success indicator such as a success toast or navigation to the new
network/policy details (e.g., assert a success alert is visible or the details
header/route for the created network appears) so the flow uses
physicalNetworkNameInput, submitBtn and the success selector instead of fixed
time delays.
In `@ui-tests-cy/views/vmNetwork.ts`:
- Line 63: Remove the bypass of Cypress actionability checks on the project
checkbox: replace the call to cy.contains('li',
projectName).find('input[type="checkbox"]').click({ force: true }) with an
actionable interaction such as cy.contains('li',
projectName).find('input[type="checkbox"]').should('be.visible').check() (or
.click() after the should('be.visible') assertion) so the test fails if the
checkbox is not interactable instead of forcing the click.
- Around line 17-31: The click using { force: true } in attemptToCreateVMNetwork
should be replaced with an actionable click that respects Cypress checks: locate
the "Create physical network" button via cy.contains('button', 'Create physical
network') and ensure it's visible and enabled (e.g.,
.should('be.visible').and('not.be.disabled')) and/or scroll into view before
calling .click(), so the flow in navigateToVMN → cy.get(itemCreateBtn) →
fillPhysicalNetworkWizard remains the same but without bypassing actionability;
adjust waits/timeouts around fillPhysicalNetworkWizard and the subsequent
navigateToVMN call if needed to make the button reliably actionable.
- Around line 41-80: Remove hardcoded cy.waits and replace them with explicit
assertions: drop the initial cy.wait(2 * SECOND) before interacting with
vmnNameInput because the existing cy.get(vmnNameInput).should('be.visible') is
sufficient; after typing into the physical network combobox (the first
'input[role="combobox"]') replace cy.wait(SECOND) with an assertion that the
expected option/button[role="option"] containing physicalNetworkName is visible
(e.g.,
cy.get('button[role="option"]').contains(physicalNetworkName).should('be.visible')
before clicking); similarly after typing into the project combobox
(input[role="combobox"][aria-label="Type to filter"]) assert that the matching
li or checkbox option for projectName is visible before clicking it; and after
clicking cy.contains(submitBtn, 'Create') remove cy.wait(5 * SECOND) and instead
wait for the post-submit success indicator or navigation (e.g., assert
visibility of a toast, a "1 project selected" update, or a unique page element)
to confirm completion.
- Around line 93-97: The three hardcoded cy.waits should be replaced with
explicit assertions: wait for the delete modal to appear (use an assertion that
the modal is visible before interacting instead of cy.wait(2 * SECOND)), replace
the post-click wait for '`#delete-vm-network-modal-acknowledge-checkbox`' with an
assertion that the checkbox is checked (e.g., assert checked after .click({
force: true })), and replace the final cy.wait(3 * SECOND) with an assertion
that the modal has closed or the deleted VM row is gone (e.g., assert the modal
is not visible or the row representing the VM no longer exists after clicking
the 'Delete' button). Use the existing selectors
'`#delete-vm-network-modal-acknowledge-checkbox`' and the button containing
'Delete' to locate elements and rely on visibility/checked/existence assertions
instead of fixed delays.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d462aef1-c158-4072-99c6-353246832e86
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonui-tests-cy/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (29)
.env.example.gitignorecleanup.shresearch-flakiness.shsetup.shtest-cypress.shui-tests-cy/.eslintrcui-tests-cy/cypress.config.jsui-tests-cy/package.jsonui-tests-cy/plugins/index.tsui-tests-cy/reporter-config.jsonui-tests-cy/support/commands.tsui-tests-cy/support/index.tsui-tests-cy/support/login.tsui-tests-cy/support/nav.tsui-tests-cy/support/selectors.tsui-tests-cy/tests/all.cy.tsui-tests-cy/tests/nmstate/nncp.cy.tsui-tests-cy/tests/nmstate/nns.cy.tsui-tests-cy/tests/nmstate/physical.cy.tsui-tests-cy/tests/nmstate/visit-pages.cy.tsui-tests-cy/tests/nmstate/vmn.cy.tsui-tests-cy/tests/setup/login.cy.tsui-tests-cy/tsconfig.jsonui-tests-cy/utils/const/base.tsui-tests-cy/views/nncp.tsui-tests-cy/views/physicalNetwork.tsui-tests-cy/views/selector-common.tsui-tests-cy/views/vmNetwork.ts
✅ Files skipped from review due to trivial changes (2)
- ui-tests-cy/reporter-config.json
- ui-tests-cy/.eslintrc
🚧 Files skipped from review as they are similar to previous changes (16)
- ui-tests-cy/tests/all.cy.ts
- ui-tests-cy/package.json
- ui-tests-cy/tsconfig.json
- ui-tests-cy/tests/setup/login.cy.ts
- ui-tests-cy/tests/nmstate/physical.cy.ts
- ui-tests-cy/support/nav.ts
- ui-tests-cy/tests/nmstate/nncp.cy.ts
- ui-tests-cy/views/selector-common.ts
- ui-tests-cy/utils/const/base.ts
- research-flakiness.sh
- .gitignore
- ui-tests-cy/support/selectors.ts
- ui-tests-cy/support/login.ts
- ui-tests-cy/support/index.ts
- ui-tests-cy/plugins/index.ts
- ui-tests-cy/views/nncp.ts
lkladnit
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (7)
setup.sh (1)
11-11:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winQuote the variable in the
oc createcommand.The second
${TEST_NS}on line 11 is unquoted, which can cause word splitting or globbing if the value contains spaces or special characters.🔧 Proposed fix
- oc get namespace "${TEST_NS}" 2>/dev/null || oc create namespace ${TEST_NS} + oc get namespace "${TEST_NS}" 2>/dev/null || oc create namespace "${TEST_NS}"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@setup.sh` at line 11, The oc create command uses an unquoted variable; update the command that runs when creating the namespace (the oc create namespace ${TEST_NS} invocation) to quote the variable (oc create namespace "${TEST_NS}") to prevent word-splitting and globbing when TEST_NS contains spaces or special characters.test-cypress.sh (1)
3-7:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winDisable xtrace while sourcing scripts that load secrets.
Line 3 enables command tracing (
set -x) before lines 6-7 sourcecleanup.shandsetup.sh, which themselves source.envcontainingBRIDGE_KUBEADMIN_PASSWORD. This can echo the password to CI logs.🔒 Proposed fix
-set -x set +e source ./cleanup.sh source ./setup.sh + +set -xAs per coding guidelines, logging must not expose passwords, tokens, or API keys.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test-cypress.sh` around lines 3 - 7, The script enables xtrace with `set -x` before sourcing `./cleanup.sh` and `./setup.sh`, which can expose secrets from their `.env` (e.g., BRIDGE_KUBEADMIN_PASSWORD); modify the script to temporarily disable xtrace around the sensitive sources: call `set +x` (or otherwise disable tracing) immediately before `source ./cleanup.sh` and `source ./setup.sh`, then restore tracing afterwards if needed (e.g., `set -x`) and keep the existing `set +e` behavior; ensure you reference the `set -x`, `set +e`, `source ./cleanup.sh`, and `source ./setup.sh` commands when making the change.Source: Coding guidelines
cleanup.sh (1)
11-12:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCleanup commands ignore the test namespace.
Lines 11-12 delete resources without specifying
-n "${TEST_NS}", so they operate in the current context namespace instead of the test namespace created bysetup.sh. This can leave test resources behind or delete unrelated resources.Additionally, line 7 is commented out, so
TEST_NShas no fallback when.envdoesn't provide it.🔧 Proposed fix
-#TEST_NS="${TEST_NS:-cy-test-ns}" +TEST_NS="${TEST_NS:-cy-test-ns}" @@ echo "Cleaning up nmstate ui test resources..." - oc delete nncp test-form-nncp test-req-nncp example test-nncp-physical-network --ignore-not-found --wait=true --timeout=180s - oc delete --ignore-not-found=true virtualmachinenetwork test-project-vmn test-labeled-vmn --wait=true --timeout=60s 2>/dev/null || true + oc delete nncp test-form-nncp test-req-nncp example test-nncp-physical-network -n "${TEST_NS}" --ignore-not-found --wait=true --timeout=180s + oc delete --ignore-not-found=true virtualmachinenetwork test-project-vmn test-labeled-vmn -n "${TEST_NS}" --wait=true --timeout=60s 2>/dev/null || true echo "Cleanup done."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cleanup.sh` around lines 11 - 12, The oc delete invocations (the lines invoking oc delete nncp ... and oc delete --ignore-not-found=true virtualmachinenetwork ...) are missing the namespace flag and thus act in the current kubectl context; update those commands to include -n "${TEST_NS}" so they explicitly target the test namespace, and ensure TEST_NS is defined (uncomment or add a fallback default assignment for TEST_NS near where .env is sourced or in setup.sh) so the cleanup always has a valid namespace to operate against.ui-tests-cy/tests/nmstate/nncp.cy.ts (1)
14-14:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winScope the create assertion to the NNCP table row.
Line 14 uses
cy.contains(NNCP_TEST_NAME)which can match any page text. Use a row-scoped assertion likecy.contains('tr', NNCP_TEST_NAME)to be consistent with lines 24 and 32.♻️ Recommended fix
- cy.contains(NNCP_TEST_NAME).should('exist'); + cy.contains('tr', NNCP_TEST_NAME).should('exist');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui-tests-cy/tests/nmstate/nncp.cy.ts` at line 14, The assertion using cy.contains(NNCP_TEST_NAME) is too broad and can match any page text; update the check to scope it to the NNCP table row by changing the selector to cy.contains('tr', NNCP_TEST_NAME) so it matches the specific table row (the existing NNCP_TEST_NAME usage on the create assertion should be replaced to mirror the row-scoped checks used at lines with similar checks).ui-tests-cy/views/vmNetwork.ts (2)
94-96:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove
{ force: true }from modal checkbox and delete button clicks.Lines 94 and 96 use
{ force: true }to click the modal checkbox and delete button. This bypasses Cypress's actionability checks and can hide real UI issues such as overlapping modals or elements not actually visible to users.♻️ Recommended fix
- cy.get('`#delete-vm-network-modal-acknowledge-checkbox`').click({ force: true }); + cy.get('`#delete-vm-network-modal-acknowledge-checkbox`').should('be.visible').click(); cy.wait(SECOND); - cy.get('button').contains('Delete').last().click({ force: true }); + cy.get('button').contains('Delete').last().should('be.enabled').click();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui-tests-cy/views/vmNetwork.ts` around lines 94 - 96, The test is bypassing Cypress actionability checks by using { force: true } on the modal checkbox (`#delete-vm-network-modal-acknowledge-checkbox`) and the Delete button (cy.get('button').contains('Delete').last()); remove the { force: true } options and instead wait/assert the modal and controls are visible before clicking (e.g., use cy.get('`#delete-vm-network-modal-acknowledge-checkbox`').should('be.visible').click() and cy.get('button').contains('Delete').last().should('be.visible').click()), or add a targeted visibility/wait assertion for the delete modal prior to these clicks so real UI issues aren’t masked.
63-63:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove
{ force: true }from checkbox click.Line 63 uses
{ force: true }to click the checkbox, bypassing Cypress's visibility and actionability checks. Tests should fail if the checkbox is not genuinely clickable.♻️ Recommended fix
- cy.contains('li', projectName).find('input[type="checkbox"]').click({ force: true }); + cy.contains('li', projectName).find('input[type="checkbox"]').should('be.visible').click();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui-tests-cy/views/vmNetwork.ts` at line 63, The test currently forces a click on the checkbox using cy.contains('li', projectName).find('input[type="checkbox"]').click({ force: true }) which bypasses Cypress actionability checks; remove the { force: true } option and instead ensure the checkbox is actionable before interacting (for example, chain .scrollIntoView() or .should('be.visible') and then call .click() or use .check() on the input). Update the statement using the same selector (cy.contains('li', projectName).find('input[type="checkbox"]')) to perform a normal click/check without force so failures surface when the checkbox isn't genuinely clickable.ui-tests-cy/support/commands.ts (1)
12-24:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCommand injection vulnerability via string interpolation.
The
deleteResourcecommand constructs shell commands by directly interpolatingkind,name, andnamespaceinto command strings without validation. This violates the coding guideline: "Command: no shell=True, os.system, or backtick exec with user input."🛡️ Proposed fix using input validation
Cypress.Commands.add('deleteResource', (kind: string, name: string, namespace?: string) => { + // Validate inputs contain only alphanumeric, dash, underscore, and dot + const safePattern = /^[a-zA-Z0-9._-]+$/; + if (!safePattern.test(kind) || !safePattern.test(name)) { + throw new Error('Invalid resource kind or name'); + } + if (namespace && !safePattern.test(namespace)) { + throw new Error('Invalid namespace'); + } + if (!namespace) { cy.exec(`oc delete --ignore-not-found=true ${kind} ${name} --wait=true --timeout=300s`, { failOnNonZeroExit: false, timeout: 5 * MINUTE, }); return; } cy.exec( `oc delete --ignore-not-found=true -n ${namespace} ${kind} ${name} --wait=true --timeout=300s`, { failOnNonZeroExit: false, timeout: 5 * MINUTE }, ); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui-tests-cy/support/commands.ts` around lines 12 - 24, In deleteResource (ui-tests-cy/support/commands.ts) avoid command injection by validating and whitelisting the inputs before interpolating into cy.exec: enforce that kind, name, and namespace match a safe regex (e.g. /^[a-z0-9A-Z.-]+(?:-[a-z0-9A-Z.-]+)*$/ or a stricter cluster resource/name pattern) or check against an explicit allowed-kinds list; if validation fails, throw or reject the command rather than running cy.exec; then build the oc delete string only from validated values (or map kind to a known literal) so no untrusted characters can inject shell operators.Source: Coding guidelines
🧹 Nitpick comments (2)
test-cypress.sh (2)
11-14: 💤 Low valueAdd a default case to handle invalid flags.
The
getoptscase statement doesn't handle invalid flags. Whilegetoptsitself will print an error, adding a*) casedocuments the expected behavior and follows shell scripting best practices.♻️ Proposed fix
case "${flag}" in g) gui=${OPTARG};; s) spec=${OPTARG};; + *) ;; esac🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test-cypress.sh` around lines 11 - 14, Add a default branch to the getopts case statement to handle invalid flags: in the existing case handling for g) gui=${OPTARG};; and s) spec=${OPTARG};; add a *) branch that prints a helpful usage/error message (e.g., to stderr) and exits with non‑zero status; update the case block around getopts so unknown options are explicitly handled and documented, referencing the case statement and the variables gui and spec.
34-35: 💤 Low valueRemove redundant directory change.
Line 34 changes to the parent directory, but line 35 immediately changes back to
ui-tests-cy. Thecd ..on line 34 serves no purpose and can be removed.♻️ Proposed fix
node --max-old-space-size=4096 ./node_modules/.bin/cypress run --env openshift=true --browser "${BRIDGE_E2E_BROWSER_NAME:=electron}" --spec "$spec" | tee ./gui-test-screenshots/build.log test_exit_code=${PIPESTATUS[0]} - cd .. - cd ui-tests-cy && npm run cypress-postreport && cd .. + npm run cypress-postreport + cd ..🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test-cypress.sh` around lines 34 - 35, The script contains a redundant directory change: the "cd .." immediately before "cd ui-tests-cy && npm run cypress-postreport && cd .." is unnecessary; open test-cypress.sh and remove the standalone "cd .." so the script directly runs the existing "cd ui-tests-cy && npm run cypress-postreport && cd .." sequence (or alternatively run npm in the subdirectory without changing directories), ensuring the surrounding flow remains correct.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test-cypress.sh`:
- Line 25: The script uses a bare "cd ui-tests-cy" which can fail and cause
subsequent "npm ci" and "mkdir" to run in the wrong directory; after the cd
command (referencing the literal cd ui-tests-cy) add explicit error handling to
detect failure and abort the script (e.g., check the exit status and print an
error before exit) so that npm ci and mkdir only run when the directory change
succeeds.
In `@ui-tests-cy/tests/nmstate/nncp.cy.ts`:
- Around line 36-38: The tests call editNNCP(NNCP_TEST_NAME) and deleteNNCP(...)
but lack outcome assertions; after editNNCP(NNCP_TEST_NAME) add a verification
that the NNCP row for NNCP_TEST_NAME reflects the expected changes (e.g., use
the same identifier NNCP_TEST_NAME to cy.get the table/row and assert the edited
field/text/value), and after deleteNNCP(...) assert the NNCP row is removed by
asserting the selector for NNCP_TEST_NAME does not exist or is not visible;
reference the helper calls editNNCP, deleteNNCP and the NNCP_TEST_NAME constant
to place these assertions immediately after those calls.
In `@ui-tests-cy/views/vmNetwork.ts`:
- Line 26: Remove the explicit force click on the "Create physical network"
button in ui-tests-cy/views/vmNetwork.ts by modifying the cy.contains('button',
'Create physical network').click({ force: true }) call to use a normal
actionability-checked click (e.g., cy.contains('button', 'Create physical
network').click()), and if flakiness occurs, add an explicit assertion or wait
(visible/should('be.visible') or cy.wait) before clicking to ensure the button
is interactable rather than bypassing Cypress checks.
---
Duplicate comments:
In `@cleanup.sh`:
- Around line 11-12: The oc delete invocations (the lines invoking oc delete
nncp ... and oc delete --ignore-not-found=true virtualmachinenetwork ...) are
missing the namespace flag and thus act in the current kubectl context; update
those commands to include -n "${TEST_NS}" so they explicitly target the test
namespace, and ensure TEST_NS is defined (uncomment or add a fallback default
assignment for TEST_NS near where .env is sourced or in setup.sh) so the cleanup
always has a valid namespace to operate against.
In `@setup.sh`:
- Line 11: The oc create command uses an unquoted variable; update the command
that runs when creating the namespace (the oc create namespace ${TEST_NS}
invocation) to quote the variable (oc create namespace "${TEST_NS}") to prevent
word-splitting and globbing when TEST_NS contains spaces or special characters.
In `@test-cypress.sh`:
- Around line 3-7: The script enables xtrace with `set -x` before sourcing
`./cleanup.sh` and `./setup.sh`, which can expose secrets from their `.env`
(e.g., BRIDGE_KUBEADMIN_PASSWORD); modify the script to temporarily disable
xtrace around the sensitive sources: call `set +x` (or otherwise disable
tracing) immediately before `source ./cleanup.sh` and `source ./setup.sh`, then
restore tracing afterwards if needed (e.g., `set -x`) and keep the existing `set
+e` behavior; ensure you reference the `set -x`, `set +e`, `source
./cleanup.sh`, and `source ./setup.sh` commands when making the change.
In `@ui-tests-cy/support/commands.ts`:
- Around line 12-24: In deleteResource (ui-tests-cy/support/commands.ts) avoid
command injection by validating and whitelisting the inputs before interpolating
into cy.exec: enforce that kind, name, and namespace match a safe regex (e.g.
/^[a-z0-9A-Z.-]+(?:-[a-z0-9A-Z.-]+)*$/ or a stricter cluster resource/name
pattern) or check against an explicit allowed-kinds list; if validation fails,
throw or reject the command rather than running cy.exec; then build the oc
delete string only from validated values (or map kind to a known literal) so no
untrusted characters can inject shell operators.
In `@ui-tests-cy/tests/nmstate/nncp.cy.ts`:
- Line 14: The assertion using cy.contains(NNCP_TEST_NAME) is too broad and can
match any page text; update the check to scope it to the NNCP table row by
changing the selector to cy.contains('tr', NNCP_TEST_NAME) so it matches the
specific table row (the existing NNCP_TEST_NAME usage on the create assertion
should be replaced to mirror the row-scoped checks used at lines with similar
checks).
In `@ui-tests-cy/views/vmNetwork.ts`:
- Around line 94-96: The test is bypassing Cypress actionability checks by using
{ force: true } on the modal checkbox
(`#delete-vm-network-modal-acknowledge-checkbox`) and the Delete button
(cy.get('button').contains('Delete').last()); remove the { force: true } options
and instead wait/assert the modal and controls are visible before clicking
(e.g., use
cy.get('`#delete-vm-network-modal-acknowledge-checkbox`').should('be.visible').click()
and cy.get('button').contains('Delete').last().should('be.visible').click()), or
add a targeted visibility/wait assertion for the delete modal prior to these
clicks so real UI issues aren’t masked.
- Line 63: The test currently forces a click on the checkbox using
cy.contains('li', projectName).find('input[type="checkbox"]').click({ force:
true }) which bypasses Cypress actionability checks; remove the { force: true }
option and instead ensure the checkbox is actionable before interacting (for
example, chain .scrollIntoView() or .should('be.visible') and then call .click()
or use .check() on the input). Update the statement using the same selector
(cy.contains('li', projectName).find('input[type="checkbox"]')) to perform a
normal click/check without force so failures surface when the checkbox isn't
genuinely clickable.
---
Nitpick comments:
In `@test-cypress.sh`:
- Around line 11-14: Add a default branch to the getopts case statement to
handle invalid flags: in the existing case handling for g) gui=${OPTARG};; and
s) spec=${OPTARG};; add a *) branch that prints a helpful usage/error message
(e.g., to stderr) and exits with non‑zero status; update the case block around
getopts so unknown options are explicitly handled and documented, referencing
the case statement and the variables gui and spec.
- Around line 34-35: The script contains a redundant directory change: the "cd
.." immediately before "cd ui-tests-cy && npm run cypress-postreport && cd .."
is unnecessary; open test-cypress.sh and remove the standalone "cd .." so the
script directly runs the existing "cd ui-tests-cy && npm run cypress-postreport
&& cd .." sequence (or alternatively run npm in the subdirectory without
changing directories), ensuring the surrounding flow remains correct.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 9dfb809c-c099-482b-a3e2-0d64c4f601cc
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonui-tests-cy/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (28)
.env.example.gitignorecleanup.shsetup.shtest-cypress.shui-tests-cy/.eslintrcui-tests-cy/cypress.config.jsui-tests-cy/package.jsonui-tests-cy/plugins/index.tsui-tests-cy/reporter-config.jsonui-tests-cy/support/commands.tsui-tests-cy/support/index.tsui-tests-cy/support/login.tsui-tests-cy/support/nav.tsui-tests-cy/support/selectors.tsui-tests-cy/tests/all.cy.tsui-tests-cy/tests/nmstate/nncp.cy.tsui-tests-cy/tests/nmstate/nns.cy.tsui-tests-cy/tests/nmstate/physical.cy.tsui-tests-cy/tests/nmstate/visit-pages.cy.tsui-tests-cy/tests/nmstate/vmn.cy.tsui-tests-cy/tests/setup/login.cy.tsui-tests-cy/tsconfig.jsonui-tests-cy/utils/const/base.tsui-tests-cy/views/nncp.tsui-tests-cy/views/physicalNetwork.tsui-tests-cy/views/selector-common.tsui-tests-cy/views/vmNetwork.ts
✅ Files skipped from review due to trivial changes (4)
- ui-tests-cy/tests/setup/login.cy.ts
- ui-tests-cy/.eslintrc
- ui-tests-cy/package.json
- ui-tests-cy/reporter-config.json
🚧 Files skipped from review as they are similar to previous changes (16)
- .gitignore
- ui-tests-cy/tsconfig.json
- ui-tests-cy/plugins/index.ts
- ui-tests-cy/support/login.ts
- ui-tests-cy/support/nav.ts
- ui-tests-cy/tests/nmstate/nns.cy.ts
- ui-tests-cy/tests/nmstate/vmn.cy.ts
- ui-tests-cy/tests/nmstate/visit-pages.cy.ts
- ui-tests-cy/cypress.config.js
- ui-tests-cy/views/selector-common.ts
- ui-tests-cy/support/index.ts
- ui-tests-cy/views/physicalNetwork.ts
- ui-tests-cy/utils/const/base.ts
- ui-tests-cy/views/nncp.ts
- ui-tests-cy/tests/nmstate/physical.cy.ts
- ui-tests-cy/support/selectors.ts
| cleanup | ||
| setup | ||
|
|
||
| cd ui-tests-cy |
There was a problem hiding this comment.
Add error handling to the cd command.
If cd ui-tests-cy fails (directory missing or inaccessible), the script continues in the wrong directory. Lines 26-27 then run npm ci and mkdir in an unintended location, corrupting the environment or producing confusing failures.
🛡️ Proposed fix
-cd ui-tests-cy
+cd ui-tests-cy || exit 1
npm ci📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| cd ui-tests-cy | |
| cd ui-tests-cy || exit 1 | |
| npm ci |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 25-25: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test-cypress.sh` at line 25, The script uses a bare "cd ui-tests-cy" which
can fail and cause subsequent "npm ci" and "mkdir" to run in the wrong
directory; after the cd command (referencing the literal cd ui-tests-cy) add
explicit error handling to detect failure and abort the script (e.g., check the
exit status and print an error before exit) so that npm ci and mkdir only run
when the directory change succeeds.
Comment on lines
+36
to
+38
| it('test Edit NNCP modal', () => { | ||
| editNNCP(NNCP_TEST_NAME); | ||
| }); |
There was a problem hiding this comment.
Add outcome assertions for edit and delete flows.
The edit test (lines 36-38) and delete test (lines 40-43) perform actions without asserting final state. Tests should verify that the edit succeeded and that the NNCP row is removed after deletion.
🧪 Recommended fix
For the edit test:
it('test Edit NNCP modal', () => {
editNNCP(NNCP_TEST_NAME);
+ cy.visitNNCP();
+ cy.contains('tr', NNCP_TEST_NAME).should('exist');
});For the delete test:
it('delete NNCP', () => {
cy.visitNNCP();
deleteNNCP(NNCP_TEST_NAME);
+ cy.contains('tr', NNCP_TEST_NAME).should('not.exist');
});Also applies to: 40-43
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ui-tests-cy/tests/nmstate/nncp.cy.ts` around lines 36 - 38, The tests call
editNNCP(NNCP_TEST_NAME) and deleteNNCP(...) but lack outcome assertions; after
editNNCP(NNCP_TEST_NAME) add a verification that the NNCP row for NNCP_TEST_NAME
reflects the expected changes (e.g., use the same identifier NNCP_TEST_NAME to
cy.get the table/row and assert the edited field/text/value), and after
deleteNNCP(...) assert the NNCP row is removed by asserting the selector for
NNCP_TEST_NAME does not exist or is not visible; reference the helper calls
editNNCP, deleteNNCP and the NNCP_TEST_NAME constant to place these assertions
immediately after those calls.
| return; | ||
| } | ||
| // Button is disabled — click "Create physical network" to create prerequisite | ||
| cy.contains('button', 'Create physical network').click({ force: true }); |
There was a problem hiding this comment.
Remove { force: true } from "Create physical network" button click.
Line 26 uses { force: true } to click the button, bypassing Cypress's actionability checks. This can hide real UI issues such as the button being covered by another element or not actually visible to users.
♻️ Recommended fix
- cy.contains('button', 'Create physical network').click({ force: true });
+ cy.contains('button', 'Create physical network').should('be.visible').click();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ui-tests-cy/views/vmNetwork.ts` at line 26, Remove the explicit force click
on the "Create physical network" button in ui-tests-cy/views/vmNetwork.ts by
modifying the cy.contains('button', 'Create physical network').click({ force:
true }) call to use a normal actionability-checked click (e.g.,
cy.contains('button', 'Create physical network').click()), and if flakiness
occurs, add an explicit assertion or wait (visible/should('be.visible') or
cy.wait) before clicking to ensure the button is interactable rather than
bypassing Cypress checks.
Migrates tests from kubevirt-ui to a standalone ui-tests-cy/ folder with its own package.json (Cypress 15). Covers NNCP CRUD, Physical network wizard, Virtual machine network lifecycle, NNS list, and topology view. Jira: CNV-87983 Co-authored-by: Cursor <cursoragent@cursor.com>
|
@lkladnit: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Migrates tests from kubevirt-ui to a standalone ui-tests-cy/ folder with its own package.json (Cypress 15). Covers NNCP CRUD, Physical network wizard, Virtual machine network lifecycle, NNS list, and topology view.
Jira: CNV-87995
CI Verification
All 22 tests pass on Jenkins: build #243 (SUCCESS)
Includes fix for JUnit XML path — copies test results to
cypress/gui-test-screenshots/where the Jenkins pipeline expects them.Summary by CodeRabbit
Tests
Chores